| Message ID | f284a4d87cde103cd78de64b8607e3c80a17e2e5.1484090585.git.luto@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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
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.
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
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;
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(-)