On 1/13/26 3:45 PM, Eelco Chaudron wrote:
>
>
> On 13 Jan 2026, at 15:34, Ilya Maximets wrote:
>
>> On 1/12/26 5:27 PM, Eelco Chaudron wrote:
>>>
>>>
>>> On 6 Jan 2026, at 12:55, Ilya Maximets wrote:
>>>
>>>> There are multiple logical and coding issues in this functionality.
>>>> They are semi-related, so it's easier to fix them all at once instead
>>>> of trying to untangle the changes into separate commits.
>>>>
>>>> The issues are:
>>>>
>>>> - The implementation of a non-masked variant of the action is putting
>>>> the base flow values into the action instead of the updated key, so
>>>> the packet never gets actually updated.
>>>>
>>>> - But this is OK, because the non-masked variant is mostly a dead
>>>> code, since 'fully_masked' is always false, as mdtype and np are always
>>>> zeroed out before calling commit_nsh(). So, the only way to hit this
>>>> code is to manually turn off support for masked set.
>>>>
>>>> - The put_nsh_key() function can be called for a mask, but it checks
>>>> the mdtype value as if it was a key regardless. And since mdtype is
>>>> always zeroed out in the mask, the match on the context is never
>>>> propagated to the datapath flow, even if added during the action
>>>> commit.
>>>>
>>>> - The key and the base are compared for being the same twice, once
>>>> in the commit_set_nsh_action() and again in commit_nsh().
>>>>
>>>> - commit_set_nsh_action() is using an assumption that mdtype is
>>>> changing in the mask whenever the mask is changing. This is fine in
>>>> the current code, but isn't how it supposed to work in general.
>>>>
>>>> - The masked variant of the action is generating a set with an empty
>>>> mask in case the context didn't change. Same for the base header.
>>>> This works in userspace, but kernel generally fails the validation on
>>>> set() with an empty mask. The attribute should just not be provided
>>>> in this case.
>>>>
>>>> - For some reason the printing function initializes the mask to an
>>>> exact match, printing out missing attributes in full as all-zero,
>>>> while they should not be printed if not present.
>>>>
>>>> - The masked variant duplicates the part of commit_masked_set_action()
>>>> twice, while this code can be re-used without much trouble.
>>>>
>>>> Fixing all the issues above and making the code look more like it
>>>> looks for other fields and header types. Completely removing the
>>>> get/put_nsh_key() functions as we should not be clearing the context
>>>> anyway. If we somehow end up with the MD1 context on an MD2 packet,
>>>> it means that we have a bug somewhere else that should be fixed.
>>>>
>>>> The tests updated to cover the problematic functionality.
>>>>
>>>> There are still more issues here. The main one is that we need to
>>>> actually probe the datapath before using this action, as not all the
>>>> kernel versions support NSH and support for set(nsh) was recently
>>>> removed entirely from the Linux kernel due to being completely wrong.
>>>> This is the reason for not adding any system tests here, as they
>>>> would fail or crash the kernel.
>>>>
>>>> Will work on probing and the re-introduction of the support in the
>>>> kernel separately.
>>>>
>>>> Fixes: 3d2fbd70bda5 ("userspace: Add support for NSH MD1 match fields")
>>>> Fixes: f59cb331c481 ("nsh: rework NSH netlink keys and actions")
>>>> Fixes: 81fdabb94dd7 ("nsh: fix nested mask for OVS_KEY_ATTR_NSH")
>>>> Signed-off-by: Ilya Maximets <[email protected]>\
>>>
>>> Thanks Ilya for fixing this mess. I have one comment below, as I got
>>> confused :)
>>>
>>> //Eelco
>>>
>>>> ---
>>>> lib/odp-util.c | 185 ++++++++++++++++++-------------------------------
>>>> tests/nsh.at | 61 +++++++++++++---
>>>> 2 files changed, 117 insertions(+), 129 deletions(-)
>>>>
>>>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>>>> index ee1868202..fbbfa426f 100644
>>>> --- a/lib/odp-util.c
>>>> +++ b/lib/odp-util.c
>>>> @@ -1050,7 +1050,7 @@ format_odp_set_nsh(struct ds *ds, const struct
>>>> nlattr *attr)
>>>> struct ovs_key_nsh nsh_mask;
>>>>
>>>> memset(&nsh, 0, sizeof nsh);
>>>> - memset(&nsh_mask, 0xff, sizeof nsh_mask);
>>>> + memset(&nsh_mask, 0, sizeof nsh_mask);
>>>>
>>>> NL_NESTED_FOR_EACH (a, left, attr) {
>>>> enum ovs_nsh_key_attr type = nl_attr_type(a);
>>>> @@ -6333,10 +6333,6 @@ static void get_arp_key(const struct flow *, struct
>>>> ovs_key_arp *);
>>>> static void put_arp_key(const struct ovs_key_arp *, struct flow *);
>>>> static void get_nd_key(const struct flow *, struct ovs_key_nd *);
>>>> static void put_nd_key(const struct ovs_key_nd *, struct flow *);
>>>> -static void get_nsh_key(const struct flow *flow, struct ovs_key_nsh *nsh,
>>>> - bool is_mask);
>>>> -static void put_nsh_key(const struct ovs_key_nsh *nsh, struct flow *flow,
>>>> - bool is_mask);
>>>>
>>>> /* These share the same layout. */
>>>> union ovs_key_tp {
>>>> @@ -7936,16 +7932,13 @@ commit_set_action(struct ofpbuf *odp_actions, enum
>>>> ovs_key_attr key_type,
>>>> nl_msg_end_nested(odp_actions, offset);
>>>> }
>>>>
>>>> -/* Masked set actions have a mask following the data within the netlink
>>>> - * attribute. The unmasked bits in the data will be cleared as the data
>>>> - * is copied to the action. */
>>>> -void
>>>> -commit_masked_set_action(struct ofpbuf *odp_actions,
>>>> - enum ovs_key_attr key_type,
>>>> - const void *key_, const void *mask_, size_t
>>>> key_size)
>>>> +/* Commit one attribute for a masked set action. Masked set actions have
>>>> + * a mask following the data within the netlink attribute. The unmasked
>>>> bits
>>>> + * in the data will be cleared as the data is copied to the attribute. */
>>>> +static void
>>>> +commit_masked_attribute(struct ofpbuf *odp_actions, int key_type,
>>>> + const void *key_, const void *mask_, size_t
>>>> key_size)
>>>> {
>>>> - size_t offset = nl_msg_start_nested(odp_actions,
>>>> - OVS_ACTION_ATTR_SET_MASKED);
>>>> char *data = nl_msg_put_unspec_uninit(odp_actions, key_type, key_size
>>>> * 2);
>>>> const char *key = key_, *mask = mask_;
>>>>
>>>> @@ -7954,6 +7947,17 @@ commit_masked_set_action(struct ofpbuf *odp_actions,
>>>> while (key_size--) {
>>>> *data++ = *key++ & *mask++;
>>>> }
>>>> +}
>>>> +
>>>> +/* A helper to commit a masked set action for a single attribute. */
>>>> +void
>>>> +commit_masked_set_action(struct ofpbuf *odp_actions,
>>>> + enum ovs_key_attr key_type,
>>>> + const void *key_, const void *mask_, size_t
>>>> key_size)
>>>> +{
>>>> + size_t offset = nl_msg_start_nested(odp_actions,
>>>> + OVS_ACTION_ATTR_SET_MASKED);
>>>> + commit_masked_attribute(odp_actions, key_type, key_, mask_, key_size);
>>>> nl_msg_end_nested(odp_actions, offset);
>>>> }
>>>>
>>>> @@ -8541,116 +8545,65 @@ commit_set_nw_action(const struct flow *flow,
>>>> struct flow *base,
>>>> return 0;
>>>> }
>>>>
>>>> -static inline void
>>>> -get_nsh_key(const struct flow *flow, struct ovs_key_nsh *nsh, bool
>>>> is_mask)
>>>> -{
>>>> - *nsh = flow->nsh;
>>>> - if (!is_mask) {
>>>> - if (nsh->mdtype != NSH_M_TYPE1) {
>>>> - memset(nsh->context, 0, sizeof(nsh->context));
>>>> - }
>>>> - }
>>>> -}
>>>> -
>>>> -static inline void
>>>> -put_nsh_key(const struct ovs_key_nsh *nsh, struct flow *flow,
>>>> - bool is_mask OVS_UNUSED)
>>>> -{
>>>> - flow->nsh = *nsh;
>>>> - if (flow->nsh.mdtype != NSH_M_TYPE1) {
>>>> - memset(flow->nsh.context, 0, sizeof(flow->nsh.context));
>>>> - }
>>>> -}
>>>> -
>>>> static bool
>>>> -commit_nsh(const struct ovs_key_nsh * flow_nsh, bool use_masked_set,
>>>> - const struct ovs_key_nsh *key, struct ovs_key_nsh *base,
>>>> - struct ovs_key_nsh *mask, size_t size,
>>>> +commit_nsh(bool use_masked_set, const struct ovs_key_nsh *key,
>>>> + struct ovs_key_nsh *base, struct ovs_key_nsh *mask, size_t
>>>> size,
>>>> struct ofpbuf *odp_actions)
>>>> {
>>>> - enum ovs_key_attr attr = OVS_KEY_ATTR_NSH;
>>>> -
>>>> - if (memcmp(key, base, size) == 0) {
>>>> + if (!memcmp(key, base, size)) {
>>>> /* Mask bits are set when we have either read or set the
>>>> corresponding
>>>> * values. Masked bits will be exact-matched, no need to set them
>>>> * if the value did not actually change. */
>>>> return false;
>>>> }
>>>>
>>>> - bool fully_masked = odp_mask_is_exact(attr, mask, size);
>>>> + bool fully_masked = odp_mask_is_exact(OVS_KEY_ATTR_NSH, mask, size);
>>>> + size_t set_ofs;
>>>>
>>>> if (use_masked_set && !fully_masked) {
>>>> - size_t nsh_key_ofs;
>>>> - struct ovs_nsh_key_base nsh_base;
>>>> - struct ovs_nsh_key_base nsh_base_mask;
>>>> - struct ovs_nsh_key_md1 md1;
>>>> - struct ovs_nsh_key_md1 md1_mask;
>>>> - size_t offset = nl_msg_start_nested(odp_actions,
>>>> - OVS_ACTION_ATTR_SET_MASKED);
>>>> -
>>>> - nsh_base.flags = key->flags;
>>>> - nsh_base.ttl = key->ttl;
>>>> - nsh_base.mdtype = key->mdtype;
>>>> - nsh_base.np = key->np;
>>>> - nsh_base.path_hdr = key->path_hdr;
>>>> -
>>>> - nsh_base_mask.flags = mask->flags;
>>>> - nsh_base_mask.ttl = mask->ttl;
>>>> - nsh_base_mask.mdtype = mask->mdtype;
>>>> - nsh_base_mask.np = mask->np;
>>>> - nsh_base_mask.path_hdr = mask->path_hdr;
>>>> -
>>>> - /* OVS_KEY_ATTR_NSH keys */
>>>> - nsh_key_ofs = nl_msg_start_nested(odp_actions, OVS_KEY_ATTR_NSH);
>>>> -
>>>> - /* put value and mask for OVS_NSH_KEY_ATTR_BASE */
>>>> - char *data = nl_msg_put_unspec_uninit(odp_actions,
>>>> - OVS_NSH_KEY_ATTR_BASE,
>>>> - 2 * sizeof(nsh_base));
>>>> - const char *lkey = (char *)&nsh_base, *lmask = (char
>>>> *)&nsh_base_mask;
>>>> - size_t lkey_size = sizeof(nsh_base);
>>>> -
>>>> - while (lkey_size--) {
>>>> - *data++ = *lkey++ & *lmask++;
>>>> - }
>>>> - lmask = (char *)&nsh_base_mask;
>>>> - memcpy(data, lmask, sizeof(nsh_base_mask));
>>>> -
>>>> - switch (key->mdtype) {
>>>> - case NSH_M_TYPE1:
>>>> - memcpy(md1.context, key->context, sizeof key->context);
>>>> - memcpy(md1_mask.context, mask->context, sizeof mask->context);
>>>> -
>>>> - /* put value and mask for OVS_NSH_KEY_ATTR_MD1 */
>>>> - data = nl_msg_put_unspec_uninit(odp_actions,
>>>> - OVS_NSH_KEY_ATTR_MD1,
>>>> - 2 * sizeof(md1));
>>>> - lkey = (char *)&md1;
>>>> - lmask = (char *)&md1_mask;
>>>> - lkey_size = sizeof(md1);
>>>> -
>>>> - while (lkey_size--) {
>>>> - *data++ = *lkey++ & *lmask++;
>>>> - }
>>>> - lmask = (char *)&md1_mask;
>>>> - memcpy(data, lmask, sizeof(md1_mask));
>>>> - break;
>>>> - case NSH_M_TYPE2:
>>>> - default:
>>>> - /* No match support for other MD formats yet. */
>>>> - break;
>>>> + struct ovs_nsh_key_base nsh_base = {
>>>> + .flags = key->flags,
>>>> + .ttl = key->ttl,
>>>> + /* 'mdtype' and 'np' are not writable. */
>>>> + .path_hdr = key->path_hdr,
>>>> + };
>>>> + struct ovs_nsh_key_base nsh_base_mask = {
>>>> + .flags = mask->flags,
>>>> + .ttl = mask->ttl,
>>>> + /* 'mdtype' and 'np' are not writable. */
>>>> + .path_hdr = mask->path_hdr,
>>>> + };
>>>> + size_t nsh_ofs;
>>>> +
>>>> + set_ofs = nl_msg_start_nested(odp_actions,
>>>> OVS_ACTION_ATTR_SET_MASKED);
>>>> + nsh_ofs = nl_msg_start_nested(odp_actions, OVS_KEY_ATTR_NSH);
>>>> +
>>>> + if (!is_all_zeros(&nsh_base_mask, sizeof nsh_base_mask)) {
>>>> + commit_masked_attribute(odp_actions, OVS_NSH_KEY_ATTR_BASE,
>>>> + &nsh_base, &nsh_base_mask,
>>>> + sizeof nsh_base);
>>>> }
>>>>
>>>> - nl_msg_end_nested(odp_actions, nsh_key_ofs);
>>>> + /* No match support for other MD formats yet. */
>>>> + if (key->mdtype == NSH_M_TYPE1
>>>> + && !is_all_zeros(mask->context, sizeof *mask->context)) {
>>>
>>> Is the sizeof correct here? This points to an array element, so it would
>>> only give 4 bytes, not 4×4. I think this is what the old code uses?
>>
>> Yeah, you're right. Should be without the star, since it's an array.
>
> ACK, so with that change;
>
> Acked-by: Eelco Chaudron <[email protected]>
Thanks! I fixed the issue up and applied the change.
Backported down to 3.3.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
