Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1555998 > unrolled thread

[PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256 Andy Lutomirski <luto@kernel.org> - 2017-01-11 00:30 +0100
    Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256 Daniel Borkmann <daniel@iogearbox.net> - 2017-01-11 02:00 +0100
      Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256 Andy Lutomirski <luto@amacapital.net> - 2017-01-11 04:20 +0100
        Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256 Daniel Borkmann <daniel@iogearbox.net> - 2017-01-11 10:20 +0100
          Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256 Andy Lutomirski <luto@amacapital.net> - 2017-01-11 19:30 +0100
            Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256 Daniel Borkmann <daniel@iogearbox.net> - 2017-01-14 00:10 +0100

#1555998 — [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256

This makes it easier to add another digest algorithm down the road if
needed.  It also serves to force any programs that might have been
written against a kernel that had the old field name to notice the
change and make any necessary changes.

This shouldn't violate any stable API policies, as no released kernel
has ever had TCA*BPF_DIGEST.

Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 include/uapi/linux/pkt_cls.h       | 2 +-
 include/uapi/linux/tc_act/tc_bpf.h | 2 +-
 net/sched/act_bpf.c                | 2 +-
 net/sched/cls_bpf.c                | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index cb4bcdc58543..ac6b300c1550 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -397,7 +397,7 @@ enum {
 	TCA_BPF_NAME,
 	TCA_BPF_FLAGS,
 	TCA_BPF_FLAGS_GEN,
-	TCA_BPF_DIGEST,
+	TCA_BPF_SHA256,
 	__TCA_BPF_MAX,
 };
 
diff --git a/include/uapi/linux/tc_act/tc_bpf.h b/include/uapi/linux/tc_act/tc_bpf.h
index a6b88a6f7f71..eae18a7430eb 100644
--- a/include/uapi/linux/tc_act/tc_bpf.h
+++ b/include/uapi/linux/tc_act/tc_bpf.h
@@ -27,7 +27,7 @@ enum {
 	TCA_ACT_BPF_FD,
 	TCA_ACT_BPF_NAME,
 	TCA_ACT_BPF_PAD,
-	TCA_ACT_BPF_DIGEST,
+	TCA_ACT_BPF_SHA256,
 	__TCA_ACT_BPF_MAX,
 };
 #define TCA_ACT_BPF_MAX (__TCA_ACT_BPF_MAX - 1)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 1c60317f0121..3868a66d0b24 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -123,7 +123,7 @@ static int tcf_bpf_dump_ebpf_info(const struct tcf_bpf *prog,
 	    nla_put_string(skb, TCA_ACT_BPF_NAME, prog->bpf_name))
 		return -EMSGSIZE;
 
-	nla = nla_reserve(skb, TCA_ACT_BPF_DIGEST,
+	nla = nla_reserve(skb, TCA_ACT_BPF_SHA256,
 			  sizeof(prog->filter->digest));
 	if (nla == NULL)
 		return -EMSGSIZE;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index adc776048d1a..6fc110321621 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -555,7 +555,7 @@ static int cls_bpf_dump_ebpf_info(const struct cls_bpf_prog *prog,
 	    nla_put_string(skb, TCA_BPF_NAME, prog->bpf_name))
 		return -EMSGSIZE;
 
-	nla = nla_reserve(skb, TCA_BPF_DIGEST, sizeof(prog->filter->digest));
+	nla = nla_reserve(skb, TCA_BPF_SHA256, sizeof(prog->filter->digest));
 	if (nla == NULL)
 		return -EMSGSIZE;
 
-- 
2.9.3

[toc] | [next] | [standalone]

#1556114

On 01/11/2017 12:24 AM, Andy Lutomirski wrote:
> This makes it easier to add another digest algorithm down the road if
> needed.  It also serves to force any programs that might have been
> written against a kernel that had the old field name to notice the
> change and make any necessary changes.
>
> This shouldn't violate any stable API policies, as no released kernel
> has ever had TCA*BPF_DIGEST.

Imho, this and patch 6/8 is not really needed. Should there ever
another digest alg be used (doubt it), then you'd need a new nl
attribute and fdinfo line anyway to keep existing stuff intact.
Nobody made the claim that you can just change this underneath
and not respecting abi for existing applications when I read from
above that such apps now will get "forced" to notice a change.

[toc] | [prev] | [next] | [standalone]

#1556177

On Tue, Jan 10, 2017 at 4:50 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 01/11/2017 12:24 AM, Andy Lutomirski wrote:
>>
>> This makes it easier to add another digest algorithm down the road if
>> needed.  It also serves to force any programs that might have been
>> written against a kernel that had the old field name to notice the
>> change and make any necessary changes.
>>
>> This shouldn't violate any stable API policies, as no released kernel
>> has ever had TCA*BPF_DIGEST.
>
>
> Imho, this and patch 6/8 is not really needed. Should there ever
> another digest alg be used (doubt it), then you'd need a new nl
> attribute and fdinfo line anyway to keep existing stuff intact.
> Nobody made the claim that you can just change this underneath
> and not respecting abi for existing applications when I read from
> above that such apps now will get "forced" to notice a change.

Fair enough.  I was more concerned about prerelease iproute2 versions,
but maybe that's a nonissue.  I'll drop these two patches.

--Andy

[toc] | [prev] | [next] | [standalone]

#1556308

Hi Andy,

On 01/11/2017 04:11 AM, Andy Lutomirski wrote:
> On Tue, Jan 10, 2017 at 4:50 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 01/11/2017 12:24 AM, Andy Lutomirski wrote:
>>>
>>> This makes it easier to add another digest algorithm down the road if
>>> needed.  It also serves to force any programs that might have been
>>> written against a kernel that had the old field name to notice the
>>> change and make any necessary changes.
>>>
>>> This shouldn't violate any stable API policies, as no released kernel
>>> has ever had TCA*BPF_DIGEST.
>>
>> Imho, this and patch 6/8 is not really needed. Should there ever
>> another digest alg be used (doubt it), then you'd need a new nl
>> attribute and fdinfo line anyway to keep existing stuff intact.
>> Nobody made the claim that you can just change this underneath
>> and not respecting abi for existing applications when I read from
>> above that such apps now will get "forced" to notice a change.
>
> Fair enough.  I was more concerned about prerelease iproute2 versions,
> but maybe that's a nonissue.  I'll drop these two patches.

Ok. Sleeping over this a bit, how about a general rename into
"prog_tag" for fdinfo and TCA_BPF_TAG resp. TCA_ACT_BPF_TAG for
the netlink attributes, fwiw, it might reduce any assumptions on
this being made? If this would be preferable, I could cook that
patch against -net for renaming it?

Thanks,
Daniel

[toc] | [prev] | [next] | [standalone]

#1556812

On Wed, Jan 11, 2017 at 1:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Hi Andy,
>
> On 01/11/2017 04:11 AM, Andy Lutomirski wrote:
>>
>> On Tue, Jan 10, 2017 at 4:50 PM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>>>
>>> On 01/11/2017 12:24 AM, Andy Lutomirski wrote:
>>>>
>>>>
>>>> This makes it easier to add another digest algorithm down the road if
>>>> needed.  It also serves to force any programs that might have been
>>>> written against a kernel that had the old field name to notice the
>>>> change and make any necessary changes.
>>>>
>>>> This shouldn't violate any stable API policies, as no released kernel
>>>> has ever had TCA*BPF_DIGEST.
>>>
>>>
>>> Imho, this and patch 6/8 is not really needed. Should there ever
>>> another digest alg be used (doubt it), then you'd need a new nl
>>> attribute and fdinfo line anyway to keep existing stuff intact.
>>> Nobody made the claim that you can just change this underneath
>>> and not respecting abi for existing applications when I read from
>>> above that such apps now will get "forced" to notice a change.
>>
>>
>> Fair enough.  I was more concerned about prerelease iproute2 versions,
>> but maybe that's a nonissue.  I'll drop these two patches.
>
>
> Ok. Sleeping over this a bit, how about a general rename into
> "prog_tag" for fdinfo and TCA_BPF_TAG resp. TCA_ACT_BPF_TAG for
> the netlink attributes, fwiw, it might reduce any assumptions on
> this being made? If this would be preferable, I could cook that
> patch against -net for renaming it?

That would be fine with me.

I think there are two reasonable approaches to computing the actual tag.

1. Use a standard, modern cryptographic hash.  SHA-256, SHA-512,
Blake2b, whatever.  SHA-1 is a bad choice in part because it's partly
broken and in part because the implementation in lib/ is a real mess
to use (as you noticed while writing the code).

2. Use whatever algorithm you like but make the tag so short that it's
obviously not collision-free.  48 or 64 bits is probably reasonable.

The intermediate versions are just asking for trouble.  Alexei wants
to make the tag shorter, but I admit I still don't understand why he
prefers that over using a better crypto hash and letting user code
truncate the tag if it wants.

[toc] | [prev] | [next] | [standalone]

#1558793

On 01/11/2017 07:19 PM, Andy Lutomirski wrote:
> On Wed, Jan 11, 2017 at 1:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
[...]
>> Ok. Sleeping over this a bit, how about a general rename into
>> "prog_tag" for fdinfo and TCA_BPF_TAG resp. TCA_ACT_BPF_TAG for
>> the netlink attributes, fwiw, it might reduce any assumptions on
>> this being made? If this would be preferable, I could cook that
>> patch against -net for renaming it?
>
> That would be fine with me.
>
> I think there are two reasonable approaches to computing the actual tag.
>
> 1. Use a standard, modern cryptographic hash.  SHA-256, SHA-512,
> Blake2b, whatever.  SHA-1 is a bad choice in part because it's partly
> broken and in part because the implementation in lib/ is a real mess
> to use (as you noticed while writing the code).
>
> 2. Use whatever algorithm you like but make the tag so short that it's
> obviously not collision-free.  48 or 64 bits is probably reasonable.
>
> The intermediate versions are just asking for trouble.

Yeah agree, I've just sent a patch to rework this a bit and it got
also reasonably small for net. Cleanups, if needed, can be done in
net-next once that's pulled into it.

Thanks,
Daniel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web