Hi Jakub,
thanks for your review.
On Mon, 14 Dec 2020 20:57:40 -0800
Jakub Kicinski <k...@kernel.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
