[PATCH net-next v2 0/2] netlink: specs: fill in the missing ovs operations

[PATCH net-next v2 0/2] netlink: specs: fill in the missing ovs operations

[Minxi Hou]

The ovs_flow spec described get and new but not del or set, and the
ovs_datapath and ovs_vport specs had no set. The kernel has registered
all three since the families were added. This series names the
operations and the attributes each handler reads, and marks the write
operations uns-admin-perm.

No kernel code changes. I checked the new operations against a live
kernel with YnlFamily before posting v1.

v2:
  - ovs_flow: probe is a flag, matching the NLA_FLAG policy. It stayed
    binary in v1 because no request listed it.
  - ovs_flow: del lists probe. ovs_flow_cmd_del() reads it to decide
    whether to log.
  - ovs_flow: the set doc says that replacing actions needs a key even
    when a ufid is present. ovs_nla_init_match_and_action() returns
    -EINVAL otherwise.

  v1: https://lore.kernel.org/netdev/20261006130533.1620706-1-houminxi@gmail.com/

Minxi Hou (2):
  netlink: specs: add ovs_flow del and set operations
  netlink: specs: add ovs datapath and vport set operations

 Documentation/netlink/specs/ovs_datapath.yaml | 18 ++++++++++
 Documentation/netlink/specs/ovs_flow.yaml     | 35 ++++++++++++++++++-
 Documentation/netlink/specs/ovs_vport.yaml    | 15 ++++++++
 3 files changed, 67 insertions(+), 1 deletion(-)

base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
Signed-off-by: Minxi Hou <houminxi@gmail.com>

-- 
2.55.0

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

[Minxi Hou]

The ovs_flow spec only described get and new, so a YNL client could
not delete or modify a flow. The kernel has had both commands since
the family was added: del flushes the data path when neither a key nor
a ufid is given, and set looks a flow up by key or ufid.

Name the attributes each command reads. del and set need CAP_NET_ADMIN.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 Documentation/netlink/specs/ovs_flow.yaml | 35 ++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 951837b72e1d..b1c884b4115d 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
         doc: |
           Flow operation is a feature probe, error logging should be suppressed.
       -
@@ -988,6 +988,36 @@ operations:
       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
+    -
+      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.
+      value: 4
+      attribute-set: flow-attrs
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -995,6 +1025,9 @@ operations:
             - ufid
             - mask
             - actions
+            - ufid-flags
+            - clear
+            - probe
 
 mcast-groups:
   list:
-- 
2.55.0

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

[netdev-bot+sashiko]

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

[PATCH net-next v2 2/2] netlink: specs: add ovs datapath and vport set operations

[Minxi Hou]

Both families register a SET command that the spec never described, so
a YNL client could not modify an existing datapath or vport. The kernel
handlers take CAP_NET_ADMIN through GENL_UNS_ADMIN_PERM.

Datapath set changes user features, the masks cache size and the
per-cpu upcall pids. Vport set changes the upcall pid; the kernel
rejects a type change and rejects options.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 Documentation/netlink/specs/ovs_datapath.yaml | 18 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 15 +++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
index f7b3671991e6..9c33d6a2f05d 100644
--- a/Documentation/netlink/specs/ovs_datapath.yaml
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -138,6 +138,7 @@ operations:
       doc: Create new OVS data path
       value: 1
       attribute-set: datapath
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -149,10 +150,27 @@ operations:
       doc: Delete existing OVS data path
       value: 2
       attribute-set: datapath
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
             - name
+    -
+      name: set
+      doc: |
+        Modify an existing OVS data path. The kernel treats a missing
+        user-features attribute as zero and clears the previously set
+        features.
+      value: 4
+      attribute-set: datapath
+      flags: [uns-admin-perm]
+      do:
+        request:
+          attributes:
+            - name
+            - user-features
+            - masks-cache-size
+            - per-cpu-pids
 
 mcast-groups:
   list:
diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
index fa975f8821b6..63a0a41a1199 100644
--- a/Documentation/netlink/specs/ovs_vport.yaml
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -128,6 +128,7 @@ operations:
       name: new
       doc: Create a new OVS vport
       attribute-set: vport
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -140,6 +141,7 @@ operations:
       name: del
       doc: Delete existing OVS vport from a data path
       attribute-set: vport
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -165,6 +167,19 @@ operations:
             - netnsid
             - upcall-stats
       dump: *vport-get-op
+    -
+      name: set
+      doc: Modify an existing OVS vport
+      value: 4
+      attribute-set: vport
+      flags: [uns-admin-perm]
+      do:
+        request:
+          attributes:
+            - name
+            - type
+            - options
+            - upcall-pid
 
 mcast-groups:
   list:
-- 
2.55.0

Re: [PATCH net-next v2 2/2] netlink: specs: add ovs datapath and vport set operations

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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 `set` operation in
  Documentation/netlink/specs/ovs_vport.yaml lists only [name, type,…
- [Low] The new ovs_vport `set` operation lists `options` as a request
  attribute, but ovs_vport_cmd_set() always fails with -EOPNOTSUPP when…
- [Low] The datapath `set` doc in
  Documentation/netlink/specs/ovs_datapath.yaml warns that a missing…

--

netlink: specs: add ovs datapath and vport set operations

This commit adds the missing set operations to the ovs_datapath and
ovs_vport netlink specs. It also marks the new, del and set operations of
both families with the uns-admin-perm flag.

> diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
> index f7b3671991e6c..9c33d6a2f05db 100644
> --- a/Documentation/netlink/specs/ovs_datapath.yaml
> +++ b/Documentation/netlink/specs/ovs_datapath.yaml

[ ... ]

> @@ -149,10 +150,27 @@ operations:
>        doc: Delete existing OVS data path
>        value: 2
>        attribute-set: datapath
> +      flags: [uns-admin-perm]
>        do:
>          request:
>            attributes:
>              - name
> +    -
> +      name: set
> +      doc: |
> +        Modify an existing OVS data path. The kernel treats a missing
> +        user-features attribute as zero and clears the previously set
> +        features.
> +      value: 4
> +      attribute-set: datapath
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - name
> +            - user-features
> +            - masks-cache-size
> +            - per-cpu-pids

[Severity: Low]
Should the doc also say that per-cpu-pids depends on user-features in the
same request?

In ovs_dp_change(), the new features are stored first, and the pids are
only applied when the per-CPU dispatch bit is set:

net/openvswitch/datapath.c:ovs_dp_change() {
    ...
	dp->user_features = user_features;

	if (dp->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU &&
	    a[OVS_DP_ATTR_PER_CPU_PIDS]) {
		/* Upcall Netlink Port IDs have been updated */
		err = ovs_dp_set_upcall_portids(dp,
						a[OVS_DP_ATTR_PER_CPU_PIDS]);
    ...
}

Consider a client that sends only name and per-cpu-pids to refresh the
per-CPU upcall pids. Per-CPU dispatch gets turned off, the pids are
ignored, and the request still returns success.

Could the doc state that per-cpu-pids only takes effect when
dispatch-upcall-per-cpu is set in user-features in the same request?

> diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
> index fa975f8821b6c..63a0a41a11997 100644
> --- a/Documentation/netlink/specs/ovs_vport.yaml
> +++ b/Documentation/netlink/specs/ovs_vport.yaml

[ ... ]

> @@ -165,6 +167,19 @@ operations:
>              - netnsid
>              - upcall-stats
>        dump: *vport-get-op
> +    -
> +      name: set
> +      doc: Modify an existing OVS vport
> +      value: 4
> +      attribute-set: vport
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - name

[Severity: Medium]
Should port-no be in this request list as well?

ovs_vport_cmd_set() finds the vport with lookup_vport(). That function
accepts either a name, or a port number together with dp_ifindex from the
fixed header:

net/openvswitch/datapath.c:lookup_vport() {
    ...
	} else if (a[OVS_VPORT_ATTR_PORT_NO]) {
		u32 port_no = nla_get_u32(a[OVS_VPORT_ATTR_PORT_NO]);
		...
		dp = get_dp(net, ovs_header->dp_ifindex);
		...
	} else
		return ERR_PTR(-EINVAL);
}

The del operation in this spec goes through the same lookup_vport() and
lists port-no. Without port-no here, the generated YNL request struct for
set has no port_no member.

That means YNL clients can only select the vport by name for set, even
though the kernel also supports lookup by datapath ifindex plus port
number.

> +            - type
> +            - options

[Severity: Low]
Is options meant to be listed here? ovs_vport_cmd_set() rejects it every
time it is present:

net/openvswitch/datapath.c:ovs_vport_cmd_set() {
    ...
	if (a[OVS_VPORT_ATTR_OPTIONS]) {
		/* There are no vport types that support legacy options. */
		err = -EOPNOTSUPP;
		goto exit_unlock_free;
	}
    ...
}

vport_policy also marks OVS_VPORT_ATTR_OPTIONS as unused.

Type can't really be modified either. It is only accepted when it matches
vport->ops->type, and any other value returns -EINVAL.

The commit message says "the kernel rejects a type change and rejects
options", but the spec's doc ("Modify an existing OVS vport") says neither.
Could options be dropped from the list, and the doc say that type has to
match the existing vport type?

> +            - upcall-pid
>  
>  mcast-groups:
>    list:

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