| Message ID | 20240703104600.455125-2-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage | expand |
From: Asbjørn Sloth Tønnesen <ast@fiberby.net> Date: Wed, 3 Jul 2024 10:45:50 +0000 > Define new TCA_FLOWER_KEY_FLAGS_* flags for use in struct > flow_dissector_key_control, covering the same flags > as currently exposed through TCA_FLOWER_KEY_ENC_FLAGS, > but assign them new bit positions in so that they don't > conflict with existing TCA_FLOWER_KEY_FLAGS_* flags. > > Synchronize FLOW_DIS_* flags, but put the new flags > under FLOW_DIS_F_*. The idea is that we can later, move > the existing flags under FLOW_DIS_F_* as well. > > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> > --- > include/net/flow_dissector.h | 17 +++++++++++++---- > include/uapi/linux/pkt_cls.h | 5 +++++ > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h > index 3e47e123934d..f560e2c8d0e7 100644 > --- a/include/net/flow_dissector.h > +++ b/include/net/flow_dissector.h > @@ -16,7 +16,8 @@ struct sk_buff; > * struct flow_dissector_key_control: > * @thoff: Transport header offset > * @addr_type: Type of key. One of FLOW_DISSECTOR_KEY_* > - * @flags: Key flags. Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAGENCAPSULATION) > + * @flags: Key flags. > + * Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAG|ENCAPSULATION|F_*) > */ > struct flow_dissector_key_control { > u16 thoff; > @@ -24,9 +25,17 @@ struct flow_dissector_key_control { > u32 flags; > }; > > -#define FLOW_DIS_IS_FRAGMENT BIT(0) > -#define FLOW_DIS_FIRST_FRAG BIT(1) > -#define FLOW_DIS_ENCAPSULATION BIT(2) > +/* Please keep these flags in sync with TCA_FLOWER_KEY_FLAGS_* > + * in include/uapi/linux/pkt_cls.h, as these bit flags are exposed > + * to userspace in some error paths, ie. unsupported flags. > + */ > +#define FLOW_DIS_IS_FRAGMENT BIT(0) > +#define FLOW_DIS_FIRST_FRAG BIT(1) > +#define FLOW_DIS_ENCAPSULATION BIT(2) > +#define FLOW_DIS_F_TUNNEL_CSUM BIT(3) > +#define FLOW_DIS_F_TUNNEL_DONT_FRAGMENT BIT(4) > +#define FLOW_DIS_F_TUNNEL_OAM BIT(5) > +#define FLOW_DIS_F_TUNNEL_CRIT_OPT BIT(6) 1. enum? Then they will be included in BTF info, maybe they might come handy later in BPF... 2. Maybe "sync" them automatically? FLOW_DIS_FIRST_FRAG = TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST, ... (with the only exception for BIT(2)) > > enum flow_dissect_ret { > FLOW_DISSECT_RET_OUT_GOOD, > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index b6d38f5fd7c0..24795aad7651 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -677,6 +677,11 @@ enum { > enum { > TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), > TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), > + /* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */ Should uAPI header contain this comment? FLOW_DIS_ENCAPSULATION is an internal kernel definition, so I believe its name shouldn't leak to the userspace header. > + TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 3), > + TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 4), > + TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 5), > + TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 6), > }; > > enum { Thanks, Olek
On Wed, 3 Jul 2024 12:59:54 +0200 Alexander Lobakin wrote: > > enum { > > TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), > > TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), > > + /* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */ > > Should uAPI header contain this comment? FLOW_DIS_ENCAPSULATION is an > internal kernel definition, so I believe its name shouldn't leak to the > userspace header. Also since it's internal, can avoid the gap in uAPI and make ENCAPSULATION be something like "last uAPI bit + 1" ?
Hi Kuba and Alexander, On 7/4/24 12:20 AM, Jakub Kicinski wrote: > On Wed, 3 Jul 2024 12:59:54 +0200 Alexander Lobakin wrote: >>> enum { >>> TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), >>> TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), >>> + /* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */ >> >> Should uAPI header contain this comment? FLOW_DIS_ENCAPSULATION is an >> internal kernel definition, so I believe its name shouldn't leak to the >> userspace header. > > Also since it's internal, can avoid the gap in uAPI and make > ENCAPSULATION be something like "last uAPI bit + 1" ? Would the below work? -#define FLOW_DIS_IS_FRAGMENT BIT(0) -#define FLOW_DIS_FIRST_FRAG BIT(1) -#define FLOW_DIS_ENCAPSULATION BIT(2) +/* The control flags are kept in sync with TCA_FLOWER_KEY_FLAGS_*, as those + * flags are exposed to userspace in some error paths, ie. unsupported flags. + */ +enum flow_dissector_ctrl_flags { + FLOW_DIS_IS_FRAGMENT = TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, + FLOW_DIS_FIRST_FRAG = TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST, + FLOW_DIS_F_TUNNEL_CSUM = TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM, + FLOW_DIS_F_TUNNEL_DONT_FRAGMENT = TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT, + FLOW_DIS_F_TUNNEL_OAM = TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM, + FLOW_DIS_F_TUNNEL_CRIT_OPT = TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT, + + /* These flags are internal to the kernel */ + FLOW_DIS_ENCAPSULATION = (TCA_FLOWER_KEY_FLAGS_MAX << 1), +};
On Thu, 4 Jul 2024 21:45:14 +0000 Asbjørn Sloth Tønnesen wrote: > + /* These flags are internal to the kernel */ > + FLOW_DIS_ENCAPSULATION = (TCA_FLOWER_KEY_FLAGS_MAX << 1), Looks good (assuming TCA_FLOWER_KEY_FLAGS_MAX == TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT)
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h index 3e47e123934d..f560e2c8d0e7 100644 --- a/include/net/flow_dissector.h +++ b/include/net/flow_dissector.h @@ -16,7 +16,8 @@ struct sk_buff; * struct flow_dissector_key_control: * @thoff: Transport header offset * @addr_type: Type of key. One of FLOW_DISSECTOR_KEY_* - * @flags: Key flags. Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAGENCAPSULATION) + * @flags: Key flags. + * Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAG|ENCAPSULATION|F_*) */ struct flow_dissector_key_control { u16 thoff; @@ -24,9 +25,17 @@ struct flow_dissector_key_control { u32 flags; }; -#define FLOW_DIS_IS_FRAGMENT BIT(0) -#define FLOW_DIS_FIRST_FRAG BIT(1) -#define FLOW_DIS_ENCAPSULATION BIT(2) +/* Please keep these flags in sync with TCA_FLOWER_KEY_FLAGS_* + * in include/uapi/linux/pkt_cls.h, as these bit flags are exposed + * to userspace in some error paths, ie. unsupported flags. + */ +#define FLOW_DIS_IS_FRAGMENT BIT(0) +#define FLOW_DIS_FIRST_FRAG BIT(1) +#define FLOW_DIS_ENCAPSULATION BIT(2) +#define FLOW_DIS_F_TUNNEL_CSUM BIT(3) +#define FLOW_DIS_F_TUNNEL_DONT_FRAGMENT BIT(4) +#define FLOW_DIS_F_TUNNEL_OAM BIT(5) +#define FLOW_DIS_F_TUNNEL_CRIT_OPT BIT(6) enum flow_dissect_ret { FLOW_DISSECT_RET_OUT_GOOD, diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index b6d38f5fd7c0..24795aad7651 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -677,6 +677,11 @@ enum { enum { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), + /* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */ + TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 3), + TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 4), + TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 5), + TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 6), }; enum {
Define new TCA_FLOWER_KEY_FLAGS_* flags for use in struct flow_dissector_key_control, covering the same flags as currently exposed through TCA_FLOWER_KEY_ENC_FLAGS, but assign them new bit positions in so that they don't conflict with existing TCA_FLOWER_KEY_FLAGS_* flags. Synchronize FLOW_DIS_* flags, but put the new flags under FLOW_DIS_F_*. The idea is that we can later, move the existing flags under FLOW_DIS_F_* as well. Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- include/net/flow_dissector.h | 17 +++++++++++++---- include/uapi/linux/pkt_cls.h | 5 +++++ 2 files changed, 18 insertions(+), 4 deletions(-)