Hi Davide,

On Thu, Feb 1, 2024 at 6:14 AM Davide Caratti [off-list ref] wrote:

hello Jamal, thanks for looking at this!

On Wed, Jan 31, 2024 at 04:13:25PM -0500, Jamal Hadi Salim wrote:

quoted

On Wed, Jan 31, 2024 at 11:16 AM Davide Caratti [off-list ref] wrote:

quoted

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 <redacted>

[...]

quoted

quoted

@@ -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,...))

quoted

+               NL_SET_ERR_MSG(extack, "missing enc_flags mask");
+               return -EINVAL;
+       }

right, I will change it in the v2.

[...]

quoted

quoted

@@ -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

quoted

+               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