Re: [PATCH net-next] seg6: fix the max number of supported SRv6 behavior attributes

[Andrea Mayer]

Hi Jakub,
thanks for your review.

On Mon, 14 Dec 2020 20:57:40 -0800
Jakub Kicinski <kuba@xxxxxxxxxx> 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