On Sat, 12 Dec 2020 02:00:05 +0100 Andrea Mayer wrote:
> The set of required attributes for a given SRv6 behavior is identified
> using a bitmap stored in an unsigned long, since the initial design of
> SRv6 networking in Linux. Recently the same approach has been used for
> identifying the optional attributes.
>
> We realized that choosing an unsigned long to store a bitmap is not a
> proper design choice considering that new attributes can be defined in the
> future. The problem is that the size of an unsigned long can be 32 or 64
> bits depending on the architecture. Therefore the maximum number of
> attributes that can be defined currently is 32 or 64 depending on the
> architecture. This issue has not caused any harm so far, because new
> attributes are defined statically in the kernel code, and currently only 10
> attributes have been defined.
>
> With this patch, we set the maximum number of attributes that can be
> supported by the SRv6 networking in Linux to be 64 regardless of the
> architecture.
>
> We changed the unsigned long type of the bitmaps to the u64 type and
> adapted the code accordingly. In particular:
>
> - We replaced all patterns (1 << i) with the macro SEG6_F_ATTR(i) in order
> to address overflow issues caused by 32-bit signed arithmetic.
>
> Many thanks to Colin Ian King for catching the overflow problem and
> inspiring this patch;
>
> - 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.
> 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.
> Signed-off-by: Andrea Mayer <andrea.ma...@uniroma2.it>
> ---
> include/uapi/linux/seg6_local.h | 10 ++++
> net/ipv6/seg6_local.c | 89 ++++++++++++++++++---------------
> 2 files changed, 60 insertions(+), 39 deletions(-)
>
> 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.
> +#define SEG6_F_ATTR(i) (((u64)1) << (i))
This wrapper looks useful, worth keeping.
> @@ -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)
> return lwtunnel_encap_add_ops(&seg6_local_ops,
> LWTUNNEL_ENCAP_SEG6_LOCAL);
> }
