lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 16 Dec 2020 02:08:34 +0100 From: Andrea Mayer <andrea.mayer@...roma2.it> To: Jakub Kicinski <kuba@...nel.org> Cc: "David S. Miller" <davem@...emloft.net>, Alexey Kuznetsov <kuznet@....inr.ac.ru>, Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>, David Lebrun <david.lebrun@...ouvain.be>, Mathieu Xhonneux <m.xhonneux@...il.com>, Daniel Borkmann <daniel@...earbox.net>, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, Colin Ian King <colin.king@...onical.com>, Stefano Salsano <stefano.salsano@...roma2.it>, Paolo Lungaroni <paolo.lungaroni@...t.it>, Ahmed Abdelsalam <ahabdels.dev@...il.com>, Andrea Mayer <andrea.mayer@...roma2.it> Subject: Re: [PATCH net-next] seg6: fix the max number of supported SRv6 behavior attributes Hi Jakub, thanks for your review. On Mon, 14 Dec 2020 20:57:40 -0800 Jakub Kicinski <kuba@...nel.org> wrote: > > > > - At compile time we verify that the total number of attributes does not > > exceed the fixed value of 64. Otherwise, kernel build fails forcing > > developers to reconsider adding a new attribute or extending the > > total number of supported attributes by the SRv6 networking. > > Over all seems like a good thing too catch but the patch seems to go > further than necessary. And on 32bit systems using u64 is when we only > need 10 attrs is kinda wasteful. > Ok, so the maximum number of supported attributes will be 32 (i.e. the minimum number of bits for an unsigned long). > > Fixes: d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel") > > Fixes: 140f04c33bbc ("ipv6: sr: implement several seg6local actions") > > Fixes: 891ef8dd2a8d ("ipv6: sr: implement additional seg6local actions") > > Fixes: 004d4b274e2a ("ipv6: sr: Add seg6local action End.BPF") > > Fixes: 964adce526a4 ("seg6: improve management of behavior attributes") > > Fixes: 0a3021f1d4e5 ("seg6: add support for optional attributes in SRv6 behaviors") > > Fixes: 664d6f86868b ("seg6: add support for the SRv6 End.DT4 behavior") > > Fixes: 20a081b7984c ("seg6: add VRF support for SRv6 End.DT6 behavior") > > We use fixes tags for bugs only, nothing seems broken here. It's more > of a fool-proofing for the future. > Ok, I got it. > > > > diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h > > index 3b39ef1dbb46..81b3ac430670 100644 > > --- a/include/uapi/linux/seg6_local.h > > +++ b/include/uapi/linux/seg6_local.h > > @@ -27,9 +27,19 @@ enum { > > SEG6_LOCAL_OIF, > > SEG6_LOCAL_BPF, > > SEG6_LOCAL_VRFTABLE, > > + /* new attributes go here */ > > __SEG6_LOCAL_MAX, > > + > > + /* Support up to 64 different types of attributes. > > + * > > + * If you need to add a new attribute, please make sure that it DOES > > + * NOT violate the constraint of having a maximum of 64 possible > > + * attributes. > > + */ > > + __SEG6_LOCAL_MAX_SUPP = 64, > > Let's not define this, especially in a uAPI header. No need to make > promises on max attr id to user space. > Ok. > > > +#define SEG6_F_ATTR(i) (((u64)1) << (i)) > > This wrapper looks useful, worth keeping. > We can go ahead with the wrapper that will become as follows: #define SEG6_F_ATTR(i) BIT(i) > > @@ -1692,6 +1694,15 @@ static const struct lwtunnel_encap_ops seg6_local_ops = { > > > > int __init seg6_local_init(void) > > { > > + /* If the max total number of defined attributes is reached, then your > > + * kernel build stops here. > > + * > > + * This check is required to avoid arithmetic overflows when processing > > + * behavior attributes and the maximum number of defined attributes > > + * exceeds the allowed value. > > + */ > > + BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > SEG6_LOCAL_MAX_SUPP); > > BUILD_BUG_ON(SEG6_LOCAL_MAX > 31) > I agree with this approach. Only for the sake of clarity I would prefer to define the macro SEG6_LOCAL_MAX_SUPP as follows: in seg6_local.c: [...] /* max total number of supported SRv6 behavior attributes */ #define SEG6_LOCAL_MAX_SUPP 32 int __init seg6_local_init(void) { BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > SEG6_LOCAL_MAX_SUPP); [...] } Due to the changes, I will submit a new patch (v1) with a more appropriate subject. The title of the new patch will most likely be: seg6: fool-proof the processing of SRv6 behavior attributes Thanks for your time, Andrea
Powered by blists - more mailing lists