| Message ID | 20220611104750.2724-3-andrea.mayer@uniroma2.it (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | seg6: add NEXT-C-SID support for SRv6 End behavior | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 25 this patch: 25 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 6 this patch: 6 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 25 this patch: 25 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 405 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi Andrea,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Andrea-Mayer/seg6-add-NEXT-C-SID-support-for-SRv6-End-behavior/20220611-185055
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 27f2533bcc6e909b85d3c1b738fa1f203ed8a835
config: csky-buildonly-randconfig-r006-20220613 (https://download.01.org/0day-ci/archive/20220614/202206141513.QKv7c89J-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5d121eb7b2b3ceb14906485b89e9c90dfa16b3c9
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andrea-Mayer/seg6-add-NEXT-C-SID-support-for-SRv6-End-behavior/20220611-185055
git checkout 5d121eb7b2b3ceb14906485b89e9c90dfa16b3c9
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash net/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from <command-line>:
net/ipv6/seg6_local.c: In function 'seg6_local_init':
>> include/linux/compiler_types.h:352:45: error: call to '__compiletime_assert_593' declared with attribute error: BUILD_BUG_ON failed: seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN, SEG6_LOCAL_LCNODE_FN_DLEN) != 0
352 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:333:25: note: in definition of macro '__compiletime_assert'
333 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:352:9: note: in expansion of macro '_compiletime_assert'
352 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
net/ipv6/seg6_local.c:2278:9: note: in expansion of macro 'BUILD_BUG_ON'
2278 | BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN,
| ^~~~~~~~~~~~
vim +/__compiletime_assert_593 +352 include/linux/compiler_types.h
eb5c2d4b45e3d2 Will Deacon 2020-07-21 338
eb5c2d4b45e3d2 Will Deacon 2020-07-21 339 #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 340 __compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 341
eb5c2d4b45e3d2 Will Deacon 2020-07-21 342 /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21 343 * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 344 * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21 345 * @msg: a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 346 *
eb5c2d4b45e3d2 Will Deacon 2020-07-21 347 * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 348 * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 349 * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21 350 */
eb5c2d4b45e3d2 Will Deacon 2020-07-21 351 #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @352 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 353
On Sat, 2022-06-11 at 12:47 +0200, Andrea Mayer wrote: > The NEXT-C-SID mechanism described in [1] offers the possibility of > encoding several SRv6 segments within a single 128 bit SID address. Such > a SID address is called a Compressed SID (C-SID) container. In this way, > the length of the SID List can be drastically reduced. > > A SID instantiated with the NEXT-C-SID flavor considers an IPv6 address > logically structured in three main blocks: i) Locator-Block; ii) > Locator-Node Function; iii) Argument. > > C-SID container > +------------------------------------------------------------------+ > > Locator-Block |Loc-Node| Argument | > > |Function| | > +------------------------------------------------------------------+ > <--------- B -----------> <- NF -> <------------- A ---------------> > > (i) The Locator-Block can be any IPv6 prefix available to the provider; > > (ii) The Locator-Node Function represents the node and the function to > be triggered when a packet is received on the node; > > (iii) The Argument carries the remaining C-SIDs in the current C-SID > container. > > The NEXT-C-SID mechanism relies on the "flavors" framework defined in > [2]. The flavors represent additional operations that can modify or > extend a subset of the existing behaviors. > > This patch introduces the support for flavors in SRv6 End behavior > implementing the NEXT-C-SID one. An SRv6 End behavior with NEXT-C-SID > flavor works as an End behavior but it is capable of processing the > compressed SID List encoded in C-SID containers. > > An SRv6 End behavior with NEXT-C-SID flavor can be configured to support > user-provided Locator-Block and Locator-Node Function lengths. In this > implementation, such lengths must be evenly divisible by 8 (i.e. must be > byte-aligned), otherwise the kernel informs the user about invalid > values with a meaningful error code and message through netlink_ext_ack. > > If Locator-Block and/or Locator-Node Function lengths are not provided > by the user during configuration of an SRv6 End behavior instance with > NEXT-C-SID flavor, the kernel will choose their default values i.e., > 32-bit Locator-Block and 16-bit Locator-Node Function. > > [1] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression > [2] - https://datatracker.ietf.org/doc/html/rfc8986 > > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> > --- > include/uapi/linux/seg6_local.h | 24 +++ > net/ipv6/seg6_local.c | 311 +++++++++++++++++++++++++++++++- > 2 files changed, 332 insertions(+), 3 deletions(-) > > diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h > index 332b18f318f8..7919940c84d0 100644 > --- a/include/uapi/linux/seg6_local.h > +++ b/include/uapi/linux/seg6_local.h > @@ -28,6 +28,7 @@ enum { > SEG6_LOCAL_BPF, > SEG6_LOCAL_VRFTABLE, > SEG6_LOCAL_COUNTERS, > + SEG6_LOCAL_FLAVORS, > __SEG6_LOCAL_MAX, > }; > #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1) > @@ -110,4 +111,27 @@ enum { > > #define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1) > > +/* SRv6 End* Flavor attributes */ > +enum { > + SEG6_LOCAL_FLV_UNSPEC, > + SEG6_LOCAL_FLV_OPERATION, > + SEG6_LOCAL_FLV_LCBLOCK_LEN, > + SEG6_LOCAL_FLV_LCNODE_FN_LEN, > + __SEG6_LOCAL_FLV_MAX, > +}; > + > +#define SEG6_LOCAL_FLV_MAX (__SEG6_LOCAL_FLV_MAX - 1) > + > +/* Designed flavor operations for SRv6 End* Behavior */ > +enum { > + SEG6_LOCAL_FLV_OP_UNSPEC, > + SEG6_LOCAL_FLV_OP_PSP, > + SEG6_LOCAL_FLV_OP_USP, > + SEG6_LOCAL_FLV_OP_USD, > + SEG6_LOCAL_FLV_OP_NEXT_CSID, > + __SEG6_LOCAL_FLV_OP_MAX > +}; > + > +#define SEG6_LOCAL_FLV_OP_MAX (__SEG6_LOCAL_FLV_OP_MAX - 1) > + > #endif > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > index 5ea51ee2ef71..eb31c6c838e3 100644 > --- a/net/ipv6/seg6_local.c > +++ b/net/ipv6/seg6_local.c > @@ -73,6 +73,39 @@ struct bpf_lwt_prog { > char *name; > }; > > +/* default length values (expressed in bits) for both Locator-Block and > + * Locator-Node Function. > + * > + * Both SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN *must* be: > + * i) greater than 0; > + * ii) evenly divisible by 8. In other terms, the lengths of the > + * Locator-Block and Locator-Node Function must be byte-aligned (we can > + * relax this constraint in the future if really needed). > + * > + * Moreover, a third condition must hold: > + * iii) SEG6_LOCAL_LCBLOCK_DLEN + SEG6_LOCAL_LCNODE_FN_DLEN <= 128. > + * > + * The correctness of SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN > + * values are checked during the kernel compilation. If the compilation stops, > + * check the value of these parameters to see if they meet conditions (i), (ii) > + * and (iii). > + */ > +#define SEG6_LOCAL_LCBLOCK_DLEN 32 > +#define SEG6_LOCAL_LCNODE_FN_DLEN 16 > + > +/* Supported Flavor operations are reported in this bitmask */ > +#define SEG6_LOCAL_FLV_SUPP_OPS (BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID)) > + > +struct seg6_flavors_info { > + /* Flavor operations */ > + __u32 flv_ops; > + > + /* Locator-Block length, expressed in bits */ > + __u8 lcblock_len; > + /* Locator-Node Function length, expressed in bits*/ > + __u8 lcnode_func_len; IMHO the above names are misleading. I suggest to use a '_bits' suffix instead. > +}; > + > enum seg6_end_dt_mode { > DT_INVALID_MODE = -EINVAL, > DT_LEGACY_MODE = 0, > @@ -136,6 +169,8 @@ struct seg6_local_lwt { > #ifdef CONFIG_NET_L3_MASTER_DEV > struct seg6_end_dt_info dt_info; > #endif > + struct seg6_flavors_info flv_info; > + > struct pcpu_seg6_local_counters __percpu *pcpu_counters; > > int headroom; > @@ -270,8 +305,50 @@ int seg6_lookup_nexthop(struct sk_buff *skb, > return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false); > } > > -/* regular endpoint function */ > -static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) > +static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo) > +{ > + return finfo->lcblock_len >> 3; > +} > + > +static __u8 seg6_flv_lcnode_func_octects(const struct seg6_flavors_info *finfo) > +{ > + return finfo->lcnode_func_len >> 3; > +} > + > +static bool seg6_next_csid_is_arg_zero(const struct in6_addr *addr, > + const struct seg6_flavors_info *finfo) > +{ > + __u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo); > + __u8 blk_octects = seg6_flv_lcblock_octects(finfo); > + __u8 arg_octects; > + int i; > + > + arg_octects = 16 - blk_octects - fnc_octects; > + for (i = 0; i < arg_octects; ++i) { > + if (addr->s6_addr[blk_octects + fnc_octects + i] != 0x00) > + return false; > + } > + > + return true; > +} > + > +/* assume that DA.Argument length > 0 */ > +static void seg6_next_csid_advance_arg(struct in6_addr *addr, > + const struct seg6_flavors_info *finfo) > +{ > + __u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo); > + __u8 blk_octects = seg6_flv_lcblock_octects(finfo); > + > + /* advance DA.Argument */ > + memmove((void *)&addr->s6_addr[blk_octects], > + (const void *)&addr->s6_addr[blk_octects + fnc_octects], > + 16 - blk_octects - fnc_octects); The void cast should not be needed > + > + memset((void *)&addr->s6_addr[16 - fnc_octects], 0x00, fnc_octects); Same here. > +} > + > +static int input_action_end_core(struct sk_buff *skb, > + struct seg6_local_lwt *slwt) > { > struct ipv6_sr_hdr *srh; > > @@ -290,6 +367,38 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) > return -EINVAL; > } > > +static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt) > +{ > + const struct seg6_flavors_info *finfo = &slwt->flv_info; > + struct in6_addr *daddr = &ipv6_hdr(skb)->daddr; > + > + if (seg6_next_csid_is_arg_zero(daddr, finfo)) > + return input_action_end_core(skb, slwt); > + > + /* update DA */ > + seg6_next_csid_advance_arg(daddr, finfo); > + > + seg6_lookup_nexthop(skb, NULL, 0); > + > + return dst_input(skb); > +} > + > +static bool seg6_next_csid_enabled(__u32 fops) > +{ > + return fops & BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID); > +} > + > +/* regular endpoint function */ > +static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) > +{ > + const struct seg6_flavors_info *finfo = &slwt->flv_info; > + > + if (seg6_next_csid_enabled(finfo->flv_ops)) > + return end_next_csid_core(skb, slwt); > + > + return input_action_end_core(skb, slwt); > +} > + > /* regular endpoint, and forward to specified nexthop */ > static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt) > { > @@ -952,7 +1061,8 @@ static struct seg6_action_desc seg6_action_table[] = { > { > .action = SEG6_LOCAL_ACTION_END, > .attrs = 0, > - .optattrs = SEG6_F_LOCAL_COUNTERS, > + .optattrs = SEG6_F_LOCAL_COUNTERS | > + SEG6_F_ATTR(SEG6_LOCAL_FLAVORS), > .input = input_action_end, > }, > { > @@ -1133,6 +1243,7 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = { > [SEG6_LOCAL_OIF] = { .type = NLA_U32 }, > [SEG6_LOCAL_BPF] = { .type = NLA_NESTED }, > [SEG6_LOCAL_COUNTERS] = { .type = NLA_NESTED }, > + [SEG6_LOCAL_FLAVORS] = { .type = NLA_NESTED }, > }; > > static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt, > @@ -1552,6 +1663,190 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt) > free_percpu(slwt->pcpu_counters); > } > > +static const > +struct nla_policy seg6_local_flavors_policy[SEG6_LOCAL_FLV_MAX + 1] = { > + [SEG6_LOCAL_FLV_OPERATION] = { .type = NLA_U32 }, > + [SEG6_LOCAL_FLV_LCBLOCK_LEN] = { .type = NLA_U8 }, > + [SEG6_LOCAL_FLV_LCNODE_FN_LEN] = { .type = NLA_U8 }, > +}; > + > +/* check whether the lengths of the Locator-Block and Locator-Node Function > + * are compatible with the dimension of a C-SID container. > + */ > +static int seg6_chk_next_csid_cfg(__u8 block_len, __u8 func_len) > +{ > + /* Locator-Block and Locator-Node Function cannot exceed 128 bits */ > + if (block_len + func_len > 128) > + return -EINVAL; > + > + /* Locator-Block length must be greater than zero and evenly divisible > + * by 8. There must be room for a Locator-Node Function, at least. > + */ > + if (block_len < 8 || block_len > 120 || (block_len & 0x07)) The 'block_len < 8' part is not needed, since you later check the 3 less significant bits can't be set and this is an unsigned number. > + return -EINVAL; > + > + /* Locator-Node Function length must be greater than zero and evenly > + * divisible by 8. There must be room for the Locator-Block. > + */ > + if (func_len < 8 || func_len > 120 || (func_len & 0x07)) > + return -EINVAL; > + > + return 0; > +} > + > +static int seg6_parse_nla_next_csid_cfg(struct nlattr **tb, > + struct seg6_flavors_info *finfo, > + struct netlink_ext_ack *extack) > +{ > + __u8 func_len = SEG6_LOCAL_LCNODE_FN_DLEN; > + __u8 block_len = SEG6_LOCAL_LCBLOCK_DLEN; > + int rc; > + > + if (tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]) > + block_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]); > + > + if (tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]) > + func_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]); > + > + rc = seg6_chk_next_csid_cfg(block_len, func_len); > + if (rc < 0) { > + NL_SET_ERR_MSG(extack, > + "Invalid Locator Block/Node Function lengths"); > + return rc; > + } > + > + finfo->lcblock_len = block_len; > + finfo->lcnode_func_len = func_len; > + > + return 0; > +} > + > +static int parse_nla_flavors(struct nlattr **attrs, struct seg6_local_lwt *slwt, > + struct netlink_ext_ack *extack) > +{ > + struct seg6_flavors_info *finfo = &slwt->flv_info; > + struct nlattr *tb[SEG6_LOCAL_FLV_MAX + 1]; > + unsigned long fops; > + int rc; > + > + rc = nla_parse_nested_deprecated(tb, SEG6_LOCAL_FLV_MAX, > + attrs[SEG6_LOCAL_FLAVORS], > + seg6_local_flavors_policy, NULL); > + if (rc < 0) > + return rc; > + > + /* this attribute MUST always be present since it represents the Flavor > + * operation(s) to carry out. > + */ > + if (!tb[SEG6_LOCAL_FLV_OPERATION]) > + return -EINVAL; > + > + fops = nla_get_u32(tb[SEG6_LOCAL_FLV_OPERATION]); > + if (~SEG6_LOCAL_FLV_SUPP_OPS & fops) { Please avoid 'yoda-style' syntax. The compilar warnings will catch the eventual mistakes this is supposed to avoid, and the conventional syntax is more readable. > + NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)"); > + return -EOPNOTSUPP; > + } > + > + finfo->flv_ops = fops; > + > + if (seg6_next_csid_enabled(fops)) { > + /* Locator-Block and Locator-Node Function lengths can be > + * provided by the user space. If not, default values are going > + * to be applied. > + */ > + rc = seg6_parse_nla_next_csid_cfg(tb, finfo, extack); > + if (rc < 0) > + return rc; > + } > + > + return 0; > +} > + > +static int seg6_fill_nla_next_csid_cfg(struct sk_buff *skb, > + struct seg6_flavors_info *finfo) > +{ > + if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCBLOCK_LEN, finfo->lcblock_len)) > + return -EMSGSIZE; > + > + if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCNODE_FN_LEN, > + finfo->lcnode_func_len)) > + return -EMSGSIZE; > + > + return 0; > +} > + > +static int put_nla_flavors(struct sk_buff *skb, struct seg6_local_lwt *slwt) > +{ > + struct seg6_flavors_info *finfo = &slwt->flv_info; > + __u32 fops = finfo->flv_ops; > + struct nlattr *nest; > + int rc; > + > + nest = nla_nest_start(skb, SEG6_LOCAL_FLAVORS); > + if (!nest) > + return -EMSGSIZE; > + > + if (nla_put_u32(skb, SEG6_LOCAL_FLV_OPERATION, fops)) { > + rc = -EMSGSIZE; > + goto err; > + } > + > + if (seg6_next_csid_enabled(fops)) { > + rc = seg6_fill_nla_next_csid_cfg(skb, finfo); > + if (rc < 0) > + goto err; > + } > + > + return nla_nest_end(skb, nest); > + > +err: > + nla_nest_cancel(skb, nest); > + return rc; > +} > + > +static int seg6_cmp_nla_next_csid_cfg(struct seg6_flavors_info *finfo_a, > + struct seg6_flavors_info *finfo_b) > +{ > + if (finfo_a->lcblock_len != finfo_b->lcblock_len) > + return 1; > + > + if (finfo_a->lcnode_func_len != finfo_b->lcnode_func_len) > + return 1; > + > + return 0; > +} > + > +static int cmp_nla_flavors(struct seg6_local_lwt *a, struct seg6_local_lwt *b) > +{ > + struct seg6_flavors_info *finfo_a = &a->flv_info; > + struct seg6_flavors_info *finfo_b = &b->flv_info; > + > + if (finfo_a->flv_ops != finfo_b->flv_ops) > + return 1; > + > + if (seg6_next_csid_enabled(finfo_a->flv_ops)) { > + if (seg6_cmp_nla_next_csid_cfg(finfo_a, finfo_b)) > + return 1; > + } > + > + return 0; > +} > + > +static int encap_size_flavors(struct seg6_local_lwt *slwt) > +{ > + struct seg6_flavors_info *finfo = &slwt->flv_info; > + int nlsize; > + > + nlsize = nla_total_size(0) + /* nest SEG6_LOCAL_FLAVORS */ > + nla_total_size(4); /* SEG6_LOCAL_FLV_OPERATION */ > + > + if (seg6_next_csid_enabled(finfo->flv_ops)) > + nlsize += nla_total_size(1) + /* SEG6_LOCAL_FLV_LCBLOCK_LEN */ > + nla_total_size(1); /* SEG6_LOCAL_FLV_LCNODE_FN_LEN */ > + > + return nlsize; > +} > + > struct seg6_action_param { > int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt, > struct netlink_ext_ack *extack); > @@ -1604,6 +1899,10 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = { > .put = put_nla_counters, > .cmp = cmp_nla_counters, > .destroy = destroy_attr_counters }, > + > + [SEG6_LOCAL_FLAVORS] = { .parse = parse_nla_flavors, > + .put = put_nla_flavors, > + .cmp = cmp_nla_flavors }, > }; > > /* call the destroy() callback (if available) for each set attribute in > @@ -1917,6 +2216,9 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt) > /* SEG6_LOCAL_CNT_ERRORS */ > nla_total_size_64bit(sizeof(__u64)); > > + if (attrs & SEG6_F_ATTR(SEG6_LOCAL_FLAVORS)) > + nlsize += encap_size_flavors(slwt); > + > return nlsize; > } > > @@ -1972,6 +2274,9 @@ int __init seg6_local_init(void) > */ > BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > BITS_PER_TYPE(unsigned long)); > > + BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN, > + SEG6_LOCAL_LCNODE_FN_DLEN) != 0); > + It looks like you are asking too much to the compiler with the above. You can possibly resort open code the relevant test here. It would be great if you could add some self-tests on top of the iproute2 support. Thanks! Paolo
Hi Paolo, Thanks for your time. Please see my answers inline. On Tue, 14 Jun 2022 12:52:42 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > On Sat, 2022-06-11 at 12:47 +0200, Andrea Mayer wrote: > > The NEXT-C-SID mechanism described in [1] offers the possibility of > > encoding several SRv6 segments within a single 128 bit SID address. Such > > a SID address is called a Compressed SID (C-SID) container. In this way, > > the length of the SID List can be drastically reduced. > > > > A SID instantiated with the NEXT-C-SID flavor considers an IPv6 address > > logically structured in three main blocks: i) Locator-Block; ii) > > Locator-Node Function; iii) Argument. > > > > C-SID container > > +------------------------------------------------------------------+ > > > Locator-Block |Loc-Node| Argument | > > > |Function| | > > +------------------------------------------------------------------+ > > <--------- B -----------> <- NF -> <------------- A ---------------> > > > > (i) The Locator-Block can be any IPv6 prefix available to the provider; > > > > (ii) The Locator-Node Function represents the node and the function to > > be triggered when a packet is received on the node; > > > > (iii) The Argument carries the remaining C-SIDs in the current C-SID > > container. > > > > The NEXT-C-SID mechanism relies on the "flavors" framework defined in > > [2]. The flavors represent additional operations that can modify or > > extend a subset of the existing behaviors. > > > > This patch introduces the support for flavors in SRv6 End behavior > > implementing the NEXT-C-SID one. An SRv6 End behavior with NEXT-C-SID > > flavor works as an End behavior but it is capable of processing the > > compressed SID List encoded in C-SID containers. > > > > An SRv6 End behavior with NEXT-C-SID flavor can be configured to support > > user-provided Locator-Block and Locator-Node Function lengths. In this > > implementation, such lengths must be evenly divisible by 8 (i.e. must be > > byte-aligned), otherwise the kernel informs the user about invalid > > values with a meaningful error code and message through netlink_ext_ack. > > > > If Locator-Block and/or Locator-Node Function lengths are not provided > > by the user during configuration of an SRv6 End behavior instance with > > NEXT-C-SID flavor, the kernel will choose their default values i.e., > > 32-bit Locator-Block and 16-bit Locator-Node Function. > > > > [1] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression > > [2] - https://datatracker.ietf.org/doc/html/rfc8986 > > > > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> > > --- > > include/uapi/linux/seg6_local.h | 24 +++ > > net/ipv6/seg6_local.c | 311 +++++++++++++++++++++++++++++++- > > 2 files changed, 332 insertions(+), 3 deletions(-) > > > > diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h > > index 332b18f318f8..7919940c84d0 100644 > > --- a/include/uapi/linux/seg6_local.h > > +++ b/include/uapi/linux/seg6_local.h > > @@ -28,6 +28,7 @@ enum { > > SEG6_LOCAL_BPF, > > SEG6_LOCAL_VRFTABLE, > > SEG6_LOCAL_COUNTERS, > > + SEG6_LOCAL_FLAVORS, > > __SEG6_LOCAL_MAX, > > }; > > #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1) > > @@ -110,4 +111,27 @@ enum { > > > > #define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1) > > > > +/* SRv6 End* Flavor attributes */ > > +enum { > > + SEG6_LOCAL_FLV_UNSPEC, > > + SEG6_LOCAL_FLV_OPERATION, > > + SEG6_LOCAL_FLV_LCBLOCK_LEN, > > + SEG6_LOCAL_FLV_LCNODE_FN_LEN, > > + __SEG6_LOCAL_FLV_MAX, > > +}; > > + > > +#define SEG6_LOCAL_FLV_MAX (__SEG6_LOCAL_FLV_MAX - 1) > > + > > +/* Designed flavor operations for SRv6 End* Behavior */ > > +enum { > > + SEG6_LOCAL_FLV_OP_UNSPEC, > > + SEG6_LOCAL_FLV_OP_PSP, > > + SEG6_LOCAL_FLV_OP_USP, > > + SEG6_LOCAL_FLV_OP_USD, > > + SEG6_LOCAL_FLV_OP_NEXT_CSID, > > + __SEG6_LOCAL_FLV_OP_MAX > > +}; > > + > > +#define SEG6_LOCAL_FLV_OP_MAX (__SEG6_LOCAL_FLV_OP_MAX - 1) > > + > > #endif > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > > index 5ea51ee2ef71..eb31c6c838e3 100644 > > --- a/net/ipv6/seg6_local.c > > +++ b/net/ipv6/seg6_local.c > > @@ -73,6 +73,39 @@ struct bpf_lwt_prog { > > char *name; > > }; > > > > +/* default length values (expressed in bits) for both Locator-Block and > > + * Locator-Node Function. > > + * > > + * Both SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN *must* be: > > + * i) greater than 0; > > + * ii) evenly divisible by 8. In other terms, the lengths of the > > + * Locator-Block and Locator-Node Function must be byte-aligned (we can > > + * relax this constraint in the future if really needed). > > + * > > + * Moreover, a third condition must hold: > > + * iii) SEG6_LOCAL_LCBLOCK_DLEN + SEG6_LOCAL_LCNODE_FN_DLEN <= 128. > > + * > > + * The correctness of SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN > > + * values are checked during the kernel compilation. If the compilation stops, > > + * check the value of these parameters to see if they meet conditions (i), (ii) > > + * and (iii). > > + */ > > +#define SEG6_LOCAL_LCBLOCK_DLEN 32 > > +#define SEG6_LOCAL_LCNODE_FN_DLEN 16 > > + > > +/* Supported Flavor operations are reported in this bitmask */ > > +#define SEG6_LOCAL_FLV_SUPP_OPS (BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID)) > > + > > +struct seg6_flavors_info { > > + /* Flavor operations */ > > + __u32 flv_ops; > > + > > + /* Locator-Block length, expressed in bits */ > > + __u8 lcblock_len; > > + /* Locator-Node Function length, expressed in bits*/ > > + __u8 lcnode_func_len; > > IMHO the above names are misleading. I suggest to use a '_bits' suffix > instead. > Ok, that looks nice to me, e.g,: lcblock_len -> lcblock_bits. I keep the other vars/macros consistent, so for instance: SEG6_LOCAL_LCBLOCK_DLEN -> SEG6_LOCAL_LCBLOCK_DBITS. We should also update labels such as SEG6_LOCAL_FLV_LCBLOCK_LEN and SEG6_LOCAL_FLV_LCNODE_FN_LEN used in netlink messages. > > +}; > > + > > enum seg6_end_dt_mode { > > DT_INVALID_MODE = -EINVAL, > > DT_LEGACY_MODE = 0, > > @@ -136,6 +169,8 @@ struct seg6_local_lwt { > > #ifdef CONFIG_NET_L3_MASTER_DEV > > struct seg6_end_dt_info dt_info; > > #endif > > + struct seg6_flavors_info flv_info; > > + > > struct pcpu_seg6_local_counters __percpu *pcpu_counters; > > > > int headroom; > > @@ -270,8 +305,50 @@ int seg6_lookup_nexthop(struct sk_buff *skb, > > return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false); > > } > > > > -/* regular endpoint function */ > > -static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) > > +static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo) > > +{ > > + return finfo->lcblock_len >> 3; > > +} > > + > > +static __u8 seg6_flv_lcnode_func_octects(const struct seg6_flavors_info *finfo) > > +{ > > + return finfo->lcnode_func_len >> 3; > > +} > > + > > +static bool seg6_next_csid_is_arg_zero(const struct in6_addr *addr, > > + const struct seg6_flavors_info *finfo) > > +{ > > + __u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo); > > + __u8 blk_octects = seg6_flv_lcblock_octects(finfo); > > + __u8 arg_octects; > > + int i; > > + > > + arg_octects = 16 - blk_octects - fnc_octects; > > + for (i = 0; i < arg_octects; ++i) { > > + if (addr->s6_addr[blk_octects + fnc_octects + i] != 0x00) > > + return false; > > + } > > + > > + return true; > > +} > > + > > +/* assume that DA.Argument length > 0 */ > > +static void seg6_next_csid_advance_arg(struct in6_addr *addr, > > + const struct seg6_flavors_info *finfo) > > +{ > > + __u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo); > > + __u8 blk_octects = seg6_flv_lcblock_octects(finfo); > > + > > + /* advance DA.Argument */ > > + memmove((void *)&addr->s6_addr[blk_octects], > > + (const void *)&addr->s6_addr[blk_octects + fnc_octects], > > + 16 - blk_octects - fnc_octects); > > The void cast should not be needed yes. > > > + > > + memset((void *)&addr->s6_addr[16 - fnc_octects], 0x00, fnc_octects); > > Same here. > yes. > > +} > > + > > +static int input_action_end_core(struct sk_buff *skb, > > + struct seg6_local_lwt *slwt) > > { > > struct ipv6_sr_hdr *srh; > > > > @@ -290,6 +367,38 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) > > return -EINVAL; > > } > > > > +static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt) > > +{ > > + const struct seg6_flavors_info *finfo = &slwt->flv_info; > > + struct in6_addr *daddr = &ipv6_hdr(skb)->daddr; > > + > > + if (seg6_next_csid_is_arg_zero(daddr, finfo)) > > + return input_action_end_core(skb, slwt); > > + > > + /* update DA */ > > + seg6_next_csid_advance_arg(daddr, finfo); > > + > > + seg6_lookup_nexthop(skb, NULL, 0); > > + > > + return dst_input(skb); > > +} > > + > > +static bool seg6_next_csid_enabled(__u32 fops) > > +{ > > + return fops & BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID); > > +} > > + > > +/* regular endpoint function */ > > +static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) > > +{ > > + const struct seg6_flavors_info *finfo = &slwt->flv_info; > > + > > + if (seg6_next_csid_enabled(finfo->flv_ops)) > > + return end_next_csid_core(skb, slwt); > > + > > + return input_action_end_core(skb, slwt); > > +} > > + > > /* regular endpoint, and forward to specified nexthop */ > > static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt) > > { > > @@ -952,7 +1061,8 @@ static struct seg6_action_desc seg6_action_table[] = { > > { > > .action = SEG6_LOCAL_ACTION_END, > > .attrs = 0, > > - .optattrs = SEG6_F_LOCAL_COUNTERS, > > + .optattrs = SEG6_F_LOCAL_COUNTERS | > > + SEG6_F_ATTR(SEG6_LOCAL_FLAVORS), > > .input = input_action_end, > > }, > > { > > @@ -1133,6 +1243,7 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = { > > [SEG6_LOCAL_OIF] = { .type = NLA_U32 }, > > [SEG6_LOCAL_BPF] = { .type = NLA_NESTED }, > > [SEG6_LOCAL_COUNTERS] = { .type = NLA_NESTED }, > > + [SEG6_LOCAL_FLAVORS] = { .type = NLA_NESTED }, > > }; > > > > static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt, > > @@ -1552,6 +1663,190 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt) > > free_percpu(slwt->pcpu_counters); > > } > > > > +static const > > +struct nla_policy seg6_local_flavors_policy[SEG6_LOCAL_FLV_MAX + 1] = { > > + [SEG6_LOCAL_FLV_OPERATION] = { .type = NLA_U32 }, > > + [SEG6_LOCAL_FLV_LCBLOCK_LEN] = { .type = NLA_U8 }, > > + [SEG6_LOCAL_FLV_LCNODE_FN_LEN] = { .type = NLA_U8 }, > > +}; > > + > > +/* check whether the lengths of the Locator-Block and Locator-Node Function > > + * are compatible with the dimension of a C-SID container. > > + */ > > +static int seg6_chk_next_csid_cfg(__u8 block_len, __u8 func_len) > > +{ > > + /* Locator-Block and Locator-Node Function cannot exceed 128 bits */ > > + if (block_len + func_len > 128) > > + return -EINVAL; > > + > > + /* Locator-Block length must be greater than zero and evenly divisible > > + * by 8. There must be room for a Locator-Node Function, at least. > > + */ > > + if (block_len < 8 || block_len > 120 || (block_len & 0x07)) > > The 'block_len < 8' part is not needed, since you later check the 3 > less significant bits can't be set and this is an unsigned number. > Ok, but I need to be sure that block_len must be different from zero. For this reason, I will replace 'block_len < 8' with 'block_len == 0'. > > + return -EINVAL; > > + > > + /* Locator-Node Function length must be greater than zero and evenly > > + * divisible by 8. There must be room for the Locator-Block. > > + */ > > + if (func_len < 8 || func_len > 120 || (func_len & 0x07)) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int seg6_parse_nla_next_csid_cfg(struct nlattr **tb, > > + struct seg6_flavors_info *finfo, > > + struct netlink_ext_ack *extack) > > +{ > > + __u8 func_len = SEG6_LOCAL_LCNODE_FN_DLEN; > > + __u8 block_len = SEG6_LOCAL_LCBLOCK_DLEN; > > + int rc; > > + > > + if (tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]) > > + block_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]); > > + > > + if (tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]) > > + func_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]); > > + > > + rc = seg6_chk_next_csid_cfg(block_len, func_len); > > + if (rc < 0) { > > + NL_SET_ERR_MSG(extack, > > + "Invalid Locator Block/Node Function lengths"); > > + return rc; > > + } > > + > > + finfo->lcblock_len = block_len; > > + finfo->lcnode_func_len = func_len; > > + > > + return 0; > > +} > > + > > +static int parse_nla_flavors(struct nlattr **attrs, struct seg6_local_lwt *slwt, > > + struct netlink_ext_ack *extack) > > +{ > > + struct seg6_flavors_info *finfo = &slwt->flv_info; > > + struct nlattr *tb[SEG6_LOCAL_FLV_MAX + 1]; > > + unsigned long fops; > > + int rc; > > + > > + rc = nla_parse_nested_deprecated(tb, SEG6_LOCAL_FLV_MAX, > > + attrs[SEG6_LOCAL_FLAVORS], > > + seg6_local_flavors_policy, NULL); > > + if (rc < 0) > > + return rc; > > + > > + /* this attribute MUST always be present since it represents the Flavor > > + * operation(s) to carry out. > > + */ > > + if (!tb[SEG6_LOCAL_FLV_OPERATION]) > > + return -EINVAL; > > + > > + fops = nla_get_u32(tb[SEG6_LOCAL_FLV_OPERATION]); > > + if (~SEG6_LOCAL_FLV_SUPP_OPS & fops) { > > Please avoid 'yoda-style' syntax. The compilar warnings will catch the > eventual mistakes this is supposed to avoid, and the conventional > syntax is more readable. > Ok, fine! > > + NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)"); > > + return -EOPNOTSUPP; > > + } > > + > > + finfo->flv_ops = fops; > > + > > + if (seg6_next_csid_enabled(fops)) { > > + /* Locator-Block and Locator-Node Function lengths can be > > + * provided by the user space. If not, default values are going > > + * to be applied. > > + */ > > + rc = seg6_parse_nla_next_csid_cfg(tb, finfo, extack); > > + if (rc < 0) > > + return rc; > > + } > > + > > + return 0; > > +} > > + > > +static int seg6_fill_nla_next_csid_cfg(struct sk_buff *skb, > > + struct seg6_flavors_info *finfo) > > +{ > > + if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCBLOCK_LEN, finfo->lcblock_len)) > > + return -EMSGSIZE; > > + > > + if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCNODE_FN_LEN, > > + finfo->lcnode_func_len)) > > + return -EMSGSIZE; > > + > > + return 0; > > +} > > + > > +static int put_nla_flavors(struct sk_buff *skb, struct seg6_local_lwt *slwt) > > +{ > > + struct seg6_flavors_info *finfo = &slwt->flv_info; > > + __u32 fops = finfo->flv_ops; > > + struct nlattr *nest; > > + int rc; > > + > > + nest = nla_nest_start(skb, SEG6_LOCAL_FLAVORS); > > + if (!nest) > > + return -EMSGSIZE; > > + > > + if (nla_put_u32(skb, SEG6_LOCAL_FLV_OPERATION, fops)) { > > + rc = -EMSGSIZE; > > + goto err; > > + } > > + > > + if (seg6_next_csid_enabled(fops)) { > > + rc = seg6_fill_nla_next_csid_cfg(skb, finfo); > > + if (rc < 0) > > + goto err; > > + } > > + > > + return nla_nest_end(skb, nest); > > + > > +err: > > + nla_nest_cancel(skb, nest); > > + return rc; > > +} > > + > > +static int seg6_cmp_nla_next_csid_cfg(struct seg6_flavors_info *finfo_a, > > + struct seg6_flavors_info *finfo_b) > > +{ > > + if (finfo_a->lcblock_len != finfo_b->lcblock_len) > > + return 1; > > + > > + if (finfo_a->lcnode_func_len != finfo_b->lcnode_func_len) > > + return 1; > > + > > + return 0; > > +} > > + > > +static int cmp_nla_flavors(struct seg6_local_lwt *a, struct seg6_local_lwt *b) > > +{ > > + struct seg6_flavors_info *finfo_a = &a->flv_info; > > + struct seg6_flavors_info *finfo_b = &b->flv_info; > > + > > + if (finfo_a->flv_ops != finfo_b->flv_ops) > > + return 1; > > + > > + if (seg6_next_csid_enabled(finfo_a->flv_ops)) { > > + if (seg6_cmp_nla_next_csid_cfg(finfo_a, finfo_b)) > > + return 1; > > + } > > + > > + return 0; > > +} > > + > > +static int encap_size_flavors(struct seg6_local_lwt *slwt) > > +{ > > + struct seg6_flavors_info *finfo = &slwt->flv_info; > > + int nlsize; > > + > > + nlsize = nla_total_size(0) + /* nest SEG6_LOCAL_FLAVORS */ > > + nla_total_size(4); /* SEG6_LOCAL_FLV_OPERATION */ > > + > > + if (seg6_next_csid_enabled(finfo->flv_ops)) > > + nlsize += nla_total_size(1) + /* SEG6_LOCAL_FLV_LCBLOCK_LEN */ > > + nla_total_size(1); /* SEG6_LOCAL_FLV_LCNODE_FN_LEN */ > > + > > + return nlsize; > > +} > > + > > struct seg6_action_param { > > int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt, > > struct netlink_ext_ack *extack); > > @@ -1604,6 +1899,10 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = { > > .put = put_nla_counters, > > .cmp = cmp_nla_counters, > > .destroy = destroy_attr_counters }, > > + > > + [SEG6_LOCAL_FLAVORS] = { .parse = parse_nla_flavors, > > + .put = put_nla_flavors, > > + .cmp = cmp_nla_flavors }, > > }; > > > > /* call the destroy() callback (if available) for each set attribute in > > @@ -1917,6 +2216,9 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt) > > /* SEG6_LOCAL_CNT_ERRORS */ > > nla_total_size_64bit(sizeof(__u64)); > > > > + if (attrs & SEG6_F_ATTR(SEG6_LOCAL_FLAVORS)) > > + nlsize += encap_size_flavors(slwt); > > + > > return nlsize; > > } > > > > @@ -1972,6 +2274,9 @@ int __init seg6_local_init(void) > > */ > > BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > BITS_PER_TYPE(unsigned long)); > > > > + BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN, > > + SEG6_LOCAL_LCNODE_FN_DLEN) != 0); > > + > > It looks like you are asking too much to the compiler with the above. Yes, indeed. Definitely, too much :-) > You can possibly resort open code the relevant test here. > Do you mean something like that? int __init_seg6_local(void) { [...] rc = seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN, SEG6_LOCAL_LCNODE_FN_DLEN); if (rc < 0) { WARN(1, "seg6: wrong default Locator-Block/Node Function lengths!"); return rc; } [...] } In this way, the developer who mangles these macros and recompiles the kernel will not receive any warning at compilation time. Later on, when the kernel is loaded, the initialization of the inet6 will fail with a log message in the dmesg. I am not sure if this is a good failure mode. A variant of this approach is to issue the warning in the dmesg but continue to load the inet6. In this case if the developer changes the values in a wrong way, the segment routing next csid will behave in a wrong way (not predictable). --- Another possibility is to skip any check, since these two macro values are not supposed to be changed; and if they are changed it is at the risk of the developer. --- There is another possibility that I am trying to implement, which is to create three small macros to use in seg6_chk_next_csid_cfg(...) and also with BUILD_BUG_ON(...). Something like that: #define __ncsid_chk_bits(blen, flen) \ ((blen) + (flen) > 128) #define __ncsid_chk_lcblock_bits(blen) \ (!(blen) || (blen) > 120 || ((blen) & 0x07)) #define __ncsid_chk_lcnode_fn_bits(flen) \ __ncsid_chk_lcblock_bits(flen) int __init_seg6_local(void) { [...] BUILD_BUG_ON(__ncsid_chk_bits(SEG6_LOCAL_LCBLOCK_DLEN, SEG6_LOCAL_LCNODE_FN_DLEN)); BUILD_BUG_ON(__ncsid_chk_lcblock_bits(SEG6_LOCAL_LCBLOCK_DLEN)); BUILD_BUG_ON(__ncsid_chk_lcnode_fn_bits(SEG6_LOCAL_LCNODE_FN_DLEN)); [...] } Since these are only and exclusively macros (and by the way they are very simple), there should be no problems at compile time. This approach looks promising to me. What do you think is the best approach to take in a case like that? > It would be great if you could add some self-tests on top of the > iproute2 support. > yes for sure ;-) I will add them to v2. > Thanks! > you're welcome! and thanks again for your help. Andrea
diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h index 332b18f318f8..7919940c84d0 100644 --- a/include/uapi/linux/seg6_local.h +++ b/include/uapi/linux/seg6_local.h @@ -28,6 +28,7 @@ enum { SEG6_LOCAL_BPF, SEG6_LOCAL_VRFTABLE, SEG6_LOCAL_COUNTERS, + SEG6_LOCAL_FLAVORS, __SEG6_LOCAL_MAX, }; #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1) @@ -110,4 +111,27 @@ enum { #define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1) +/* SRv6 End* Flavor attributes */ +enum { + SEG6_LOCAL_FLV_UNSPEC, + SEG6_LOCAL_FLV_OPERATION, + SEG6_LOCAL_FLV_LCBLOCK_LEN, + SEG6_LOCAL_FLV_LCNODE_FN_LEN, + __SEG6_LOCAL_FLV_MAX, +}; + +#define SEG6_LOCAL_FLV_MAX (__SEG6_LOCAL_FLV_MAX - 1) + +/* Designed flavor operations for SRv6 End* Behavior */ +enum { + SEG6_LOCAL_FLV_OP_UNSPEC, + SEG6_LOCAL_FLV_OP_PSP, + SEG6_LOCAL_FLV_OP_USP, + SEG6_LOCAL_FLV_OP_USD, + SEG6_LOCAL_FLV_OP_NEXT_CSID, + __SEG6_LOCAL_FLV_OP_MAX +}; + +#define SEG6_LOCAL_FLV_OP_MAX (__SEG6_LOCAL_FLV_OP_MAX - 1) + #endif diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c index 5ea51ee2ef71..eb31c6c838e3 100644 --- a/net/ipv6/seg6_local.c +++ b/net/ipv6/seg6_local.c @@ -73,6 +73,39 @@ struct bpf_lwt_prog { char *name; }; +/* default length values (expressed in bits) for both Locator-Block and + * Locator-Node Function. + * + * Both SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN *must* be: + * i) greater than 0; + * ii) evenly divisible by 8. In other terms, the lengths of the + * Locator-Block and Locator-Node Function must be byte-aligned (we can + * relax this constraint in the future if really needed). + * + * Moreover, a third condition must hold: + * iii) SEG6_LOCAL_LCBLOCK_DLEN + SEG6_LOCAL_LCNODE_FN_DLEN <= 128. + * + * The correctness of SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN + * values are checked during the kernel compilation. If the compilation stops, + * check the value of these parameters to see if they meet conditions (i), (ii) + * and (iii). + */ +#define SEG6_LOCAL_LCBLOCK_DLEN 32 +#define SEG6_LOCAL_LCNODE_FN_DLEN 16 + +/* Supported Flavor operations are reported in this bitmask */ +#define SEG6_LOCAL_FLV_SUPP_OPS (BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID)) + +struct seg6_flavors_info { + /* Flavor operations */ + __u32 flv_ops; + + /* Locator-Block length, expressed in bits */ + __u8 lcblock_len; + /* Locator-Node Function length, expressed in bits*/ + __u8 lcnode_func_len; +}; + enum seg6_end_dt_mode { DT_INVALID_MODE = -EINVAL, DT_LEGACY_MODE = 0, @@ -136,6 +169,8 @@ struct seg6_local_lwt { #ifdef CONFIG_NET_L3_MASTER_DEV struct seg6_end_dt_info dt_info; #endif + struct seg6_flavors_info flv_info; + struct pcpu_seg6_local_counters __percpu *pcpu_counters; int headroom; @@ -270,8 +305,50 @@ int seg6_lookup_nexthop(struct sk_buff *skb, return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false); } -/* regular endpoint function */ -static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) +static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo) +{ + return finfo->lcblock_len >> 3; +} + +static __u8 seg6_flv_lcnode_func_octects(const struct seg6_flavors_info *finfo) +{ + return finfo->lcnode_func_len >> 3; +} + +static bool seg6_next_csid_is_arg_zero(const struct in6_addr *addr, + const struct seg6_flavors_info *finfo) +{ + __u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo); + __u8 blk_octects = seg6_flv_lcblock_octects(finfo); + __u8 arg_octects; + int i; + + arg_octects = 16 - blk_octects - fnc_octects; + for (i = 0; i < arg_octects; ++i) { + if (addr->s6_addr[blk_octects + fnc_octects + i] != 0x00) + return false; + } + + return true; +} + +/* assume that DA.Argument length > 0 */ +static void seg6_next_csid_advance_arg(struct in6_addr *addr, + const struct seg6_flavors_info *finfo) +{ + __u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo); + __u8 blk_octects = seg6_flv_lcblock_octects(finfo); + + /* advance DA.Argument */ + memmove((void *)&addr->s6_addr[blk_octects], + (const void *)&addr->s6_addr[blk_octects + fnc_octects], + 16 - blk_octects - fnc_octects); + + memset((void *)&addr->s6_addr[16 - fnc_octects], 0x00, fnc_octects); +} + +static int input_action_end_core(struct sk_buff *skb, + struct seg6_local_lwt *slwt) { struct ipv6_sr_hdr *srh; @@ -290,6 +367,38 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) return -EINVAL; } +static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt) +{ + const struct seg6_flavors_info *finfo = &slwt->flv_info; + struct in6_addr *daddr = &ipv6_hdr(skb)->daddr; + + if (seg6_next_csid_is_arg_zero(daddr, finfo)) + return input_action_end_core(skb, slwt); + + /* update DA */ + seg6_next_csid_advance_arg(daddr, finfo); + + seg6_lookup_nexthop(skb, NULL, 0); + + return dst_input(skb); +} + +static bool seg6_next_csid_enabled(__u32 fops) +{ + return fops & BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID); +} + +/* regular endpoint function */ +static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt) +{ + const struct seg6_flavors_info *finfo = &slwt->flv_info; + + if (seg6_next_csid_enabled(finfo->flv_ops)) + return end_next_csid_core(skb, slwt); + + return input_action_end_core(skb, slwt); +} + /* regular endpoint, and forward to specified nexthop */ static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt) { @@ -952,7 +1061,8 @@ static struct seg6_action_desc seg6_action_table[] = { { .action = SEG6_LOCAL_ACTION_END, .attrs = 0, - .optattrs = SEG6_F_LOCAL_COUNTERS, + .optattrs = SEG6_F_LOCAL_COUNTERS | + SEG6_F_ATTR(SEG6_LOCAL_FLAVORS), .input = input_action_end, }, { @@ -1133,6 +1243,7 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = { [SEG6_LOCAL_OIF] = { .type = NLA_U32 }, [SEG6_LOCAL_BPF] = { .type = NLA_NESTED }, [SEG6_LOCAL_COUNTERS] = { .type = NLA_NESTED }, + [SEG6_LOCAL_FLAVORS] = { .type = NLA_NESTED }, }; static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt, @@ -1552,6 +1663,190 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt) free_percpu(slwt->pcpu_counters); } +static const +struct nla_policy seg6_local_flavors_policy[SEG6_LOCAL_FLV_MAX + 1] = { + [SEG6_LOCAL_FLV_OPERATION] = { .type = NLA_U32 }, + [SEG6_LOCAL_FLV_LCBLOCK_LEN] = { .type = NLA_U8 }, + [SEG6_LOCAL_FLV_LCNODE_FN_LEN] = { .type = NLA_U8 }, +}; + +/* check whether the lengths of the Locator-Block and Locator-Node Function + * are compatible with the dimension of a C-SID container. + */ +static int seg6_chk_next_csid_cfg(__u8 block_len, __u8 func_len) +{ + /* Locator-Block and Locator-Node Function cannot exceed 128 bits */ + if (block_len + func_len > 128) + return -EINVAL; + + /* Locator-Block length must be greater than zero and evenly divisible + * by 8. There must be room for a Locator-Node Function, at least. + */ + if (block_len < 8 || block_len > 120 || (block_len & 0x07)) + return -EINVAL; + + /* Locator-Node Function length must be greater than zero and evenly + * divisible by 8. There must be room for the Locator-Block. + */ + if (func_len < 8 || func_len > 120 || (func_len & 0x07)) + return -EINVAL; + + return 0; +} + +static int seg6_parse_nla_next_csid_cfg(struct nlattr **tb, + struct seg6_flavors_info *finfo, + struct netlink_ext_ack *extack) +{ + __u8 func_len = SEG6_LOCAL_LCNODE_FN_DLEN; + __u8 block_len = SEG6_LOCAL_LCBLOCK_DLEN; + int rc; + + if (tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]) + block_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]); + + if (tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]) + func_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]); + + rc = seg6_chk_next_csid_cfg(block_len, func_len); + if (rc < 0) { + NL_SET_ERR_MSG(extack, + "Invalid Locator Block/Node Function lengths"); + return rc; + } + + finfo->lcblock_len = block_len; + finfo->lcnode_func_len = func_len; + + return 0; +} + +static int parse_nla_flavors(struct nlattr **attrs, struct seg6_local_lwt *slwt, + struct netlink_ext_ack *extack) +{ + struct seg6_flavors_info *finfo = &slwt->flv_info; + struct nlattr *tb[SEG6_LOCAL_FLV_MAX + 1]; + unsigned long fops; + int rc; + + rc = nla_parse_nested_deprecated(tb, SEG6_LOCAL_FLV_MAX, + attrs[SEG6_LOCAL_FLAVORS], + seg6_local_flavors_policy, NULL); + if (rc < 0) + return rc; + + /* this attribute MUST always be present since it represents the Flavor + * operation(s) to carry out. + */ + if (!tb[SEG6_LOCAL_FLV_OPERATION]) + return -EINVAL; + + fops = nla_get_u32(tb[SEG6_LOCAL_FLV_OPERATION]); + if (~SEG6_LOCAL_FLV_SUPP_OPS & fops) { + NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)"); + return -EOPNOTSUPP; + } + + finfo->flv_ops = fops; + + if (seg6_next_csid_enabled(fops)) { + /* Locator-Block and Locator-Node Function lengths can be + * provided by the user space. If not, default values are going + * to be applied. + */ + rc = seg6_parse_nla_next_csid_cfg(tb, finfo, extack); + if (rc < 0) + return rc; + } + + return 0; +} + +static int seg6_fill_nla_next_csid_cfg(struct sk_buff *skb, + struct seg6_flavors_info *finfo) +{ + if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCBLOCK_LEN, finfo->lcblock_len)) + return -EMSGSIZE; + + if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCNODE_FN_LEN, + finfo->lcnode_func_len)) + return -EMSGSIZE; + + return 0; +} + +static int put_nla_flavors(struct sk_buff *skb, struct seg6_local_lwt *slwt) +{ + struct seg6_flavors_info *finfo = &slwt->flv_info; + __u32 fops = finfo->flv_ops; + struct nlattr *nest; + int rc; + + nest = nla_nest_start(skb, SEG6_LOCAL_FLAVORS); + if (!nest) + return -EMSGSIZE; + + if (nla_put_u32(skb, SEG6_LOCAL_FLV_OPERATION, fops)) { + rc = -EMSGSIZE; + goto err; + } + + if (seg6_next_csid_enabled(fops)) { + rc = seg6_fill_nla_next_csid_cfg(skb, finfo); + if (rc < 0) + goto err; + } + + return nla_nest_end(skb, nest); + +err: + nla_nest_cancel(skb, nest); + return rc; +} + +static int seg6_cmp_nla_next_csid_cfg(struct seg6_flavors_info *finfo_a, + struct seg6_flavors_info *finfo_b) +{ + if (finfo_a->lcblock_len != finfo_b->lcblock_len) + return 1; + + if (finfo_a->lcnode_func_len != finfo_b->lcnode_func_len) + return 1; + + return 0; +} + +static int cmp_nla_flavors(struct seg6_local_lwt *a, struct seg6_local_lwt *b) +{ + struct seg6_flavors_info *finfo_a = &a->flv_info; + struct seg6_flavors_info *finfo_b = &b->flv_info; + + if (finfo_a->flv_ops != finfo_b->flv_ops) + return 1; + + if (seg6_next_csid_enabled(finfo_a->flv_ops)) { + if (seg6_cmp_nla_next_csid_cfg(finfo_a, finfo_b)) + return 1; + } + + return 0; +} + +static int encap_size_flavors(struct seg6_local_lwt *slwt) +{ + struct seg6_flavors_info *finfo = &slwt->flv_info; + int nlsize; + + nlsize = nla_total_size(0) + /* nest SEG6_LOCAL_FLAVORS */ + nla_total_size(4); /* SEG6_LOCAL_FLV_OPERATION */ + + if (seg6_next_csid_enabled(finfo->flv_ops)) + nlsize += nla_total_size(1) + /* SEG6_LOCAL_FLV_LCBLOCK_LEN */ + nla_total_size(1); /* SEG6_LOCAL_FLV_LCNODE_FN_LEN */ + + return nlsize; +} + struct seg6_action_param { int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt, struct netlink_ext_ack *extack); @@ -1604,6 +1899,10 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = { .put = put_nla_counters, .cmp = cmp_nla_counters, .destroy = destroy_attr_counters }, + + [SEG6_LOCAL_FLAVORS] = { .parse = parse_nla_flavors, + .put = put_nla_flavors, + .cmp = cmp_nla_flavors }, }; /* call the destroy() callback (if available) for each set attribute in @@ -1917,6 +2216,9 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt) /* SEG6_LOCAL_CNT_ERRORS */ nla_total_size_64bit(sizeof(__u64)); + if (attrs & SEG6_F_ATTR(SEG6_LOCAL_FLAVORS)) + nlsize += encap_size_flavors(slwt); + return nlsize; } @@ -1972,6 +2274,9 @@ int __init seg6_local_init(void) */ BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > BITS_PER_TYPE(unsigned long)); + BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN, + SEG6_LOCAL_LCNODE_FN_DLEN) != 0); + return lwtunnel_encap_add_ops(&seg6_local_ops, LWTUNNEL_ENCAP_SEG6_LOCAL); }
The NEXT-C-SID mechanism described in [1] offers the possibility of encoding several SRv6 segments within a single 128 bit SID address. Such a SID address is called a Compressed SID (C-SID) container. In this way, the length of the SID List can be drastically reduced. A SID instantiated with the NEXT-C-SID flavor considers an IPv6 address logically structured in three main blocks: i) Locator-Block; ii) Locator-Node Function; iii) Argument. C-SID container +------------------------------------------------------------------+ | Locator-Block |Loc-Node| Argument | | |Function| | +------------------------------------------------------------------+ <--------- B -----------> <- NF -> <------------- A ---------------> (i) The Locator-Block can be any IPv6 prefix available to the provider; (ii) The Locator-Node Function represents the node and the function to be triggered when a packet is received on the node; (iii) The Argument carries the remaining C-SIDs in the current C-SID container. The NEXT-C-SID mechanism relies on the "flavors" framework defined in [2]. The flavors represent additional operations that can modify or extend a subset of the existing behaviors. This patch introduces the support for flavors in SRv6 End behavior implementing the NEXT-C-SID one. An SRv6 End behavior with NEXT-C-SID flavor works as an End behavior but it is capable of processing the compressed SID List encoded in C-SID containers. An SRv6 End behavior with NEXT-C-SID flavor can be configured to support user-provided Locator-Block and Locator-Node Function lengths. In this implementation, such lengths must be evenly divisible by 8 (i.e. must be byte-aligned), otherwise the kernel informs the user about invalid values with a meaningful error code and message through netlink_ext_ack. If Locator-Block and/or Locator-Node Function lengths are not provided by the user during configuration of an SRv6 End behavior instance with NEXT-C-SID flavor, the kernel will choose their default values i.e., 32-bit Locator-Block and 16-bit Locator-Node Function. [1] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression [2] - https://datatracker.ietf.org/doc/html/rfc8986 Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> --- include/uapi/linux/seg6_local.h | 24 +++ net/ipv6/seg6_local.c | 311 +++++++++++++++++++++++++++++++- 2 files changed, 332 insertions(+), 3 deletions(-)