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]>
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
