lkml.org 
[lkml]   [2020]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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

\
 
 \ /
  Last update: 2020-12-16 02:11    [W:0.077 / U:1.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site