Re: [PATCH net-next v6 1/2] netlink: specs: add ovs_flow del and set operations

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new ufid text in Documentation/netlink/specs/ovs_flow.yaml
  (flow-attrs/ufid) and in include/uapi/linux/openvswitch.h…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the get
  reply list, now anchored as &flow-get-reply and reused for the dump…

--

netlink: specs: add ovs_flow del and set operations

This adds the del and set operations to the ovs_flow YNL spec. It also
gives the dump its own request list, changes probe to a flag, and marks
the ops dont-validate and admin-only. The key, actions and ufid docs are
rewritten in both the spec and include/uapi/linux/openvswitch.h.

> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 951837b72e1d2..bfdbf55bc219b 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml

[ ... ]

> @@ -438,18 +446,22 @@ attribute-sets:

[ ... ]

>        -
>          name: ufid
>          type: binary
> +        checks:
> +          min-len: 1
>          doc: |
>            A value between 1-16 octets specifying a unique identifier for the
>            flow. Causes the flow to be indexed by this value rather than the
> -          value of the OVS_FLOW_ATTR_KEY attribute. Optional for all
> -          requests. Present in notifications if the flow was created with this
> -          attribute.
> +          value of the OVS_FLOW_ATTR_KEY attribute. A request that carries a
> +          value outside that range is treated as if it carried no ufid, so a
> +          new flow is indexed by its key. Optional for all requests. Present
> +          in notifications if the flow was created with this attribute.
> +          Ignored in a dump.

[Severity: Medium]
Is this accurate for a zero-length ufid? flow_policy in
net/openvswitch/datapath.c has:

    [OVS_FLOW_ATTR_UFID] = { .type = NLA_UNSPEC, .len = 1 },

and validate_nla() in lib/nlattr.c checks the minimum length even when
strict validation is skipped:

    case NLA_UNSPEC:
        if (validate & NL_VALIDATE_UNSPEC) {
            ...
        }
        if (attrlen < pt->len)
            goto out_err;

So for new, del, set and get, the genl core returns -ERANGE for a 0-byte
ufid before any handler runs. A del with a 0-byte ufid and no key then
gets -ERANGE and does not flush the data path.

For dumps the core skips validation. However, ovs_flow_cmd_dump() parses
with the same policy and returns the error:

    err = genlmsg_parse_deprecated(cb->nlh, &dp_flow_genl_family, a,
                                   OVS_FLOW_ATTR_MAX, flow_policy, NULL);
    if (err)
        return err;

So a 0-byte ufid makes the whole dump fail. It is not ignored.

The treated-as-absent path in get_ufid_len() in
net/openvswitch/flow_netlink.c is:

    if (len < 1 || len > MAX_UFID_LENGTH) {

After policy validation, only the len > 16 case can reach it.

The min-len: 1 check added just above also says 0 octets is rejected,
which contradicts the doc text under it. The commit message has the same
contradiction:

  "Anything outside 1-16 octets is read as if the attribute were absent,
  so a del with such a ufid and no key flushes the data path"

next to "record the minimum length the policy enforces in the attribute".

The same claim also appears in these places:

  - the del op doc ("A request that carries no key and no valid ufid
    flushes the data path")
  - the new key doc
  - @OVS_FLOW_ATTR_UFID and @OVS_FLOW_ATTR_KEY in
    include/uapi/linux/openvswitch.h

The later patch in the series ("netlink: specs: add ovs datapath and
vport set operations") does not touch ovs_flow.yaml or openvswitch.h, so
this text is still there at the end of the series.

Could the docs say that a value longer than 16 octets is treated as
absent, and that a zero-length value is rejected with -ERANGE?

>          display-hint: uuid

[ ... ]

> @@ -969,25 +981,33 @@ operations:
>        doc: Get / dump OVS flow configuration and state
>        value: 3
>        attribute-set: flow-attrs
> -      do: &flow-get-op
> +      dont-validate: [strict, dump]
> +      do:
>          request:
>            attributes:
>              - key
>              - ufid
>              - ufid-flags
> -        reply:
> +            - probe
> +        reply: &flow-get-reply
>            attributes:
>              - key
>              - ufid
>              - mask
>              - stats
>              - actions
> -      dump: *flow-get-op
> +      dump:
> +        request:
> +          attributes:
> +            - ufid-flags
> +        reply: *flow-get-reply

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. This
reply list is now anchored and reused for the dump, so should it also
list used and tcp-flags?

ovs_flow_cmd_fill_info() always calls ovs_flow_cmd_fill_stats(). That
function puts both attributes in get replies and dump entries:

    if (used &&
        nla_put_u64_64bit(skb, OVS_FLOW_ATTR_USED, ovs_flow_used_time(used),
                          OVS_FLOW_ATTR_PAD))
        return -EMSGSIZE;
    ...
    if ((u8)ntohs(tcp_flags) &&
         nla_put_u8(skb, OVS_FLOW_ATTR_TCP_FLAGS, (u8)ntohs(tcp_flags)))
        return -EMSGSIZE;

Because they are not in the reply list, YNL-generated clients do not
parse the last-used time or the TCP flags. The dump already shared this
reply list through *flow-get-op before this patch.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107120647.874906-1-houminxi%40gmail.com