Re: [PATCH net-next v2 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch changes two existing spec definitions in
  Documentation/netlink/specs/ovs_flow.yaml, and the commit message…

Pre-existing issues:
- [Medium] The 'new' operation's request list in
  Documentation/netlink/specs/ovs_flow.yaml is still [key, ufid, mask,…
- [Low] The flow-attrs 'key' attribute doc in
  Documentation/netlink/specs/ovs_flow.yaml (lines 386-387) says key is…
- [Low] The 'get' operation in Documentation/netlink/specs/ovs_flow.yaml
  shares one request list between do and dump (&flow-get-op = [key,…

--

netlink: specs: add ovs_flow del and set operations

This commit adds del and set operations to the ovs_flow YNL spec. Each one
gets a request attribute list and the uns-admin-perm flag. The patch also
changes the probe attribute type from binary to flag, and adds
uns-admin-perm to the existing new operation.

> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 951837b72e1d2..b1c884b4115df 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -438,7 +438,7 @@ attribute-sets:
>            are exact match bits.
>        -
>          name: probe
> -        type: binary
> +        type: flag

[Severity: Low]

This isn't a bug, but the commit message doesn't mention this change. Every
op that uses flow-attrs shares the probe attribute. It has been binary since
commit 93b230b549bc ("netlink: specs: add ynl spec for ovs_flow").

The new type matches the kernel policy in net/openvswitch/datapath.c:

    [OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },

It does change how existing YNL clients encode probe, though. In
YnlFamily._add_attr() in tools/net/ynl/pyynl/lib/ynl.py, a binary attribute
with a None or b'' value is sent as a zero-length attribute. A flag with a
falsy value is left out entirely.

The existing new operation also gains flags: [uns-admin-perm] further down.
The commit message only says:

    del and set need CAP_NET_ADMIN.

That flag does match the GENL_UNS_ADMIN_PERM set on OVS_FLOW_CMD_NEW in the
kernel.

Could both changes be described in the commit message? Alternatively, the
probe type fix could go in its own patch with a
Fixes: 93b230b549bc ("netlink: specs: add ynl spec for ovs_flow") tag.

>          doc: |
>            Flow operation is a feature probe, error logging should be suppressed.
>        -
> @@ -988,6 +988,36 @@ operations:

[Severity: Low]

This is a pre-existing issue, but the get operation just above this hunk
uses one request list for both do and dump:

      do: &flow-get-op
        request:
          attributes:
            - key
            - ufid
            - ufid-flags
      ...
      dump: *flow-get-op

ovs_flow_cmd_get() also reads probe, but probe is not in the list:

    bool log = !a[OVS_FLOW_ATTR_PROBE];

ovs_flow_cmd_dump() only reads OVS_FLOW_ATTR_UFID_FLAGS:

    ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);

So the kernel ignores the key and ufid listed in the dump request.

Should probe be added to the get do request? Should dump get its own request
list with only ufid-flags?

>        doc: Create OVS flow configuration in a data path
>        value: 1
>        attribute-set: flow-attrs
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - key
> +            - ufid
> +            - mask
> +            - actions

[Severity: Medium]

This is a pre-existing issue, but the new request list still leaves out
ufid-flags and probe. ovs_flow_cmd_new() reads both:

    u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
    ...
    bool log = !a[OVS_FLOW_ATTR_PROBE];

The del and set lists in this patch include them. Without them here, a YNL C
client generated from this spec can't send probe or ufid-flags on a NEW
request.

This patch already edits the new operation. Could these two attributes be
added to its request list too?

> +    -
> +      name: del
> +      doc: Delete one flow, or every flow in the data path
> +      value: 2
> +      attribute-set: flow-attrs
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - key
> +            - ufid
> +            - ufid-flags
> +            - probe
> +    -
> +      name: set
> +      doc: |
> +        Modify an existing flow. The kernel rejects the request with
> +        -EINVAL when it carries neither a key nor a ufid. Replacing the
> +        actions also needs a key: a ufid on its own is not enough.

[Severity: Low]

This is a pre-existing issue, but the key attribute doc in flow-attrs
contradicts these new del and set docs:

        name: key
        ...
        doc: |
          Nested attributes specifying the flow key. Always present in
          notifications. Required for all requests (except dumps).

The kernel-doc for OVS_FLOW_ATTR_KEY in include/uapi/linux/openvswitch.h has
the same wording.

The kernel accepts requests without a key. ovs_flow_cmd_del() flushes the
table when there is neither a key nor a ufid:

    if (unlikely(!a[OVS_FLOW_ATTR_KEY] && !ufid_present)) {
        err = ovs_flow_tbl_flush(&dp->table);

ovs_flow_cmd_set() and ovs_flow_cmd_get() also accept a ufid on its own:

    if (!a[OVS_FLOW_ATTR_KEY] && !ufid_present) {

Should the key doc be updated along with the new del and set docs?

[ ... ]

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