| Message ID | 91b858e0551f900a415b2d6ed80a54d7f5ef3c33.1706714667.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: allow dissecting/matching tunnel control flags | expand |
On Wed, Jan 31, 2024 at 11:16 AM Davide Caratti <dcaratti@redhat.com> wrote: > > extend cls_flower to match flags belonging to 'TUNNEL_FLAGS_PRESENT' mask > inside skb tunnel metadata. > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > include/uapi/linux/pkt_cls.h | 3 +++ > net/sched/cls_flower.c | 45 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index ea277039f89d..e3394f9d06b7 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -554,6 +554,9 @@ enum { > TCA_FLOWER_KEY_SPI, /* be32 */ > TCA_FLOWER_KEY_SPI_MASK, /* be32 */ > > + TCA_FLOWER_KEY_ENC_FLAGS, /* be16 */ > + TCA_FLOWER_KEY_ENC_FLAGS_MASK, /* be16 */ > + > __TCA_FLOWER_MAX, > }; > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index efb9d2811b73..d244169c8471 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -74,6 +74,7 @@ struct fl_flow_key { > struct flow_dissector_key_l2tpv3 l2tpv3; > struct flow_dissector_key_ipsec ipsec; > struct flow_dissector_key_cfm cfm; > + struct flow_dissector_key_enc_flags enc_flags; > } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ > > struct fl_flow_mask_range { > @@ -731,6 +732,10 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { > [TCA_FLOWER_KEY_SPI_MASK] = { .type = NLA_U32 }, > [TCA_FLOWER_L2_MISS] = NLA_POLICY_MAX(NLA_U8, 1), > [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, > + [TCA_FLOWER_KEY_ENC_FLAGS] = NLA_POLICY_MASK(NLA_BE16, > + TUNNEL_FLAGS_PRESENT), > + [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = NLA_POLICY_MASK(NLA_BE16, > + TUNNEL_FLAGS_PRESENT), > }; > > static const struct nla_policy > @@ -1748,6 +1753,21 @@ static int fl_set_key_cfm(struct nlattr **tb, > return 0; > } > > +static int fl_set_key_enc_flags(struct nlattr **tb, __be16 *flags_key, > + __be16 *flags_mask, struct netlink_ext_ack *extack) > +{ > + /* mask is mandatory for flags */ > + if (!tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]) { if (NL_REQ_ATTR_CHECK(extack,...)) > + NL_SET_ERR_MSG(extack, "missing enc_flags mask"); > + return -EINVAL; > + } > + > + *flags_key = nla_get_be16(tb[TCA_FLOWER_KEY_ENC_FLAGS]); > + *flags_mask = nla_get_be16(tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]); > + > + return 0; > +} > + > static int fl_set_key(struct net *net, struct nlattr **tb, > struct fl_flow_key *key, struct fl_flow_key *mask, > struct netlink_ext_ack *extack) > @@ -1986,6 +2006,10 @@ static int fl_set_key(struct net *net, struct nlattr **tb, > ret = fl_set_key_flags(tb, &key->control.flags, > &mask->control.flags, extack); > > + if (tb[TCA_FLOWER_KEY_ENC_FLAGS]) And here.. cheers, jamal > + ret = fl_set_key_enc_flags(tb, &key->enc_flags.flags, > + &mask->enc_flags.flags, extack); > + > return ret; > } > > @@ -2098,6 +2122,8 @@ static void fl_init_dissector(struct flow_dissector *dissector, > FLOW_DISSECTOR_KEY_IPSEC, ipsec); > FL_KEY_SET_IF_MASKED(mask, keys, cnt, > FLOW_DISSECTOR_KEY_CFM, cfm); > + FL_KEY_SET_IF_MASKED(mask, keys, cnt, > + FLOW_DISSECTOR_KEY_ENC_FLAGS, enc_flags); > > skb_flow_dissector_init(dissector, keys, cnt); > } > @@ -3185,6 +3211,22 @@ static int fl_dump_key_cfm(struct sk_buff *skb, > return err; > } > > +static int fl_dump_key_enc_flags(struct sk_buff *skb, > + struct flow_dissector_key_enc_flags *key, > + struct flow_dissector_key_enc_flags *mask) > +{ > + if (!memchr_inv(mask, 0, sizeof(*mask))) > + return 0; > + > + if (nla_put_be16(skb, TCA_FLOWER_KEY_ENC_FLAGS, key->flags)) > + return -EMSGSIZE; > + > + if (nla_put_be16(skb, TCA_FLOWER_KEY_ENC_FLAGS_MASK, mask->flags)) > + return -EMSGSIZE; > + > + return 0; > +} > + > static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type, > struct flow_dissector_key_enc_opts *enc_opts) > { > @@ -3481,6 +3523,9 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, > if (fl_dump_key_cfm(skb, &key->cfm, &mask->cfm)) > goto nla_put_failure; > > + if (fl_dump_key_enc_flags(skb, &key->enc_flags, &mask->enc_flags)) > + goto nla_put_failure; > + > return 0; > > nla_put_failure: > -- > 2.43.0 >
hello Jamal, thanks for looking at this! On Wed, Jan 31, 2024 at 04:13:25PM -0500, Jamal Hadi Salim wrote: > On Wed, Jan 31, 2024 at 11:16 AM Davide Caratti <dcaratti@redhat.com> wrote: > > > > extend cls_flower to match flags belonging to 'TUNNEL_FLAGS_PRESENT' mask > > inside skb tunnel metadata. > > > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> [...] > > @@ -1748,6 +1753,21 @@ static int fl_set_key_cfm(struct nlattr **tb, > > return 0; > > } > > > > +static int fl_set_key_enc_flags(struct nlattr **tb, __be16 *flags_key, > > + __be16 *flags_mask, struct netlink_ext_ack *extack) > > +{ > > + /* mask is mandatory for flags */ > > + if (!tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]) { > > if (NL_REQ_ATTR_CHECK(extack,...)) > > > + NL_SET_ERR_MSG(extack, "missing enc_flags mask"); > > + return -EINVAL; > > + } right, I will change it in the v2. [...] > > @@ -1986,6 +2006,10 @@ static int fl_set_key(struct net *net, struct nlattr **tb, > > ret = fl_set_key_flags(tb, &key->control.flags, > > &mask->control.flags, extack); > > > > + if (tb[TCA_FLOWER_KEY_ENC_FLAGS]) > > And here.. > > cheers, > jamal > > > + ret = fl_set_key_enc_flags(tb, &key->enc_flags.flags, > > + &mask->enc_flags.flags, extack); > > + > > return ret; here I don't see any advantage in doing if (!NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_FLOWER_KEY_ENC_FLAGS)) ret = fl_set_key_enc_flags(tb, ... ); return ret; the attribute is not mandatory, so a call to NL_SET_ERR_ATTR_MISS() would do a useless/misleading assignment in extack->miss_type. However, thanks for bringing the attention here :) At a second look, this hunk introduces a bug: in case the parsing of TCA_FLOWER_KEY_FLAGS fails, 'ret' is -EINVAL. If attributes TCA_FLOWER_KEY_ENC_FLAGS + TCA_FLOWER_KEY_ENC_FLAGS_MASK are good to go, 'ret' will be overwritten with 0 and flower will accept the rule... this is not intentional :) will fix this in the v2.
Hi Davide, On Thu, Feb 1, 2024 at 6:14 AM Davide Caratti <dcaratti@redhat.com> wrote: > > hello Jamal, thanks for looking at this! > > On Wed, Jan 31, 2024 at 04:13:25PM -0500, Jamal Hadi Salim wrote: > > On Wed, Jan 31, 2024 at 11:16 AM Davide Caratti <dcaratti@redhat.com> wrote: > > > > > > extend cls_flower to match flags belonging to 'TUNNEL_FLAGS_PRESENT' mask > > > inside skb tunnel metadata. > > > > > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > [...] > > > > @@ -1748,6 +1753,21 @@ static int fl_set_key_cfm(struct nlattr **tb, > > > return 0; > > > } > > > > > > +static int fl_set_key_enc_flags(struct nlattr **tb, __be16 *flags_key, > > > + __be16 *flags_mask, struct netlink_ext_ack *extack) > > > +{ > > > + /* mask is mandatory for flags */ > > > + if (!tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]) { > > > > if (NL_REQ_ATTR_CHECK(extack,...)) > > > > > + NL_SET_ERR_MSG(extack, "missing enc_flags mask"); > > > + return -EINVAL; > > > + } > > right, I will change it in the v2. > > [...] > > > > @@ -1986,6 +2006,10 @@ static int fl_set_key(struct net *net, struct nlattr **tb, > > > ret = fl_set_key_flags(tb, &key->control.flags, > > > &mask->control.flags, extack); > > > > > > + if (tb[TCA_FLOWER_KEY_ENC_FLAGS]) > > > > And here.. > > > > cheers, > > jamal > > > > > + ret = fl_set_key_enc_flags(tb, &key->enc_flags.flags, > > > + &mask->enc_flags.flags, extack); > > > + > > > return ret; > > here I don't see any advantage in doing > > if (!NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_FLOWER_KEY_ENC_FLAGS)) > ret = fl_set_key_enc_flags(tb, ... ); > > return ret; > Ok, i was a little overzealous there. When i see tb[] my brain goes NL_REQ_ATTR_CHECK ;-> > the attribute is not mandatory, so a call to NL_SET_ERR_ATTR_MISS() > would do a useless/misleading assignment in extack->miss_type. > True. > However, thanks for bringing the attention here :) At a second look, > this hunk introduces a bug: in case the parsing of TCA_FLOWER_KEY_FLAGS > fails, 'ret' is -EINVAL. If attributes TCA_FLOWER_KEY_ENC_FLAGS + > TCA_FLOWER_KEY_ENC_FLAGS_MASK are good to go, 'ret' will be overwritten > with 0 and flower will accept the rule... this is not intentional :) > will fix this in the v2. > np ;-> cheers, jamal > -- > davide > >
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index ea277039f89d..e3394f9d06b7 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -554,6 +554,9 @@ enum { TCA_FLOWER_KEY_SPI, /* be32 */ TCA_FLOWER_KEY_SPI_MASK, /* be32 */ + TCA_FLOWER_KEY_ENC_FLAGS, /* be16 */ + TCA_FLOWER_KEY_ENC_FLAGS_MASK, /* be16 */ + __TCA_FLOWER_MAX, }; diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index efb9d2811b73..d244169c8471 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -74,6 +74,7 @@ struct fl_flow_key { struct flow_dissector_key_l2tpv3 l2tpv3; struct flow_dissector_key_ipsec ipsec; struct flow_dissector_key_cfm cfm; + struct flow_dissector_key_enc_flags enc_flags; } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ struct fl_flow_mask_range { @@ -731,6 +732,10 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = { [TCA_FLOWER_KEY_SPI_MASK] = { .type = NLA_U32 }, [TCA_FLOWER_L2_MISS] = NLA_POLICY_MAX(NLA_U8, 1), [TCA_FLOWER_KEY_CFM] = { .type = NLA_NESTED }, + [TCA_FLOWER_KEY_ENC_FLAGS] = NLA_POLICY_MASK(NLA_BE16, + TUNNEL_FLAGS_PRESENT), + [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = NLA_POLICY_MASK(NLA_BE16, + TUNNEL_FLAGS_PRESENT), }; static const struct nla_policy @@ -1748,6 +1753,21 @@ static int fl_set_key_cfm(struct nlattr **tb, return 0; } +static int fl_set_key_enc_flags(struct nlattr **tb, __be16 *flags_key, + __be16 *flags_mask, struct netlink_ext_ack *extack) +{ + /* mask is mandatory for flags */ + if (!tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]) { + NL_SET_ERR_MSG(extack, "missing enc_flags mask"); + return -EINVAL; + } + + *flags_key = nla_get_be16(tb[TCA_FLOWER_KEY_ENC_FLAGS]); + *flags_mask = nla_get_be16(tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]); + + return 0; +} + static int fl_set_key(struct net *net, struct nlattr **tb, struct fl_flow_key *key, struct fl_flow_key *mask, struct netlink_ext_ack *extack) @@ -1986,6 +2006,10 @@ static int fl_set_key(struct net *net, struct nlattr **tb, ret = fl_set_key_flags(tb, &key->control.flags, &mask->control.flags, extack); + if (tb[TCA_FLOWER_KEY_ENC_FLAGS]) + ret = fl_set_key_enc_flags(tb, &key->enc_flags.flags, + &mask->enc_flags.flags, extack); + return ret; } @@ -2098,6 +2122,8 @@ static void fl_init_dissector(struct flow_dissector *dissector, FLOW_DISSECTOR_KEY_IPSEC, ipsec); FL_KEY_SET_IF_MASKED(mask, keys, cnt, FLOW_DISSECTOR_KEY_CFM, cfm); + FL_KEY_SET_IF_MASKED(mask, keys, cnt, + FLOW_DISSECTOR_KEY_ENC_FLAGS, enc_flags); skb_flow_dissector_init(dissector, keys, cnt); } @@ -3185,6 +3211,22 @@ static int fl_dump_key_cfm(struct sk_buff *skb, return err; } +static int fl_dump_key_enc_flags(struct sk_buff *skb, + struct flow_dissector_key_enc_flags *key, + struct flow_dissector_key_enc_flags *mask) +{ + if (!memchr_inv(mask, 0, sizeof(*mask))) + return 0; + + if (nla_put_be16(skb, TCA_FLOWER_KEY_ENC_FLAGS, key->flags)) + return -EMSGSIZE; + + if (nla_put_be16(skb, TCA_FLOWER_KEY_ENC_FLAGS_MASK, mask->flags)) + return -EMSGSIZE; + + return 0; +} + static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type, struct flow_dissector_key_enc_opts *enc_opts) { @@ -3481,6 +3523,9 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net, if (fl_dump_key_cfm(skb, &key->cfm, &mask->cfm)) goto nla_put_failure; + if (fl_dump_key_enc_flags(skb, &key->enc_flags, &mask->enc_flags)) + goto nla_put_failure; + return 0; nla_put_failure:
extend cls_flower to match flags belonging to 'TUNNEL_FLAGS_PRESENT' mask inside skb tunnel metadata. Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- include/uapi/linux/pkt_cls.h | 3 +++ net/sched/cls_flower.c | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)