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

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

[Minxi Hou]

The ovs datapath, vport and flow specs describe families the kernel
has shipped for a long time, but each one stops short of the write
commands. ovs_flow has get and new only. The datapath and vport specs
have no set. Nothing in the tree generates code from these files. A YNL
client only hits the gap when it tries to send the command.

Name the missing operations and the attributes the kernel handlers
read. del and set on ovs_flow, set on ovs_datapath and ovs_vport.

The handlers take CAP_NET_ADMIN through GENL_UNS_ADMIN_PERM, and so do
the new and del commands that were already in the specs. Mark all nine
write operations uns-admin-perm. get stays open.

Two requests are rejected for reasons the attribute list cannot say.
Flow set returns -EINVAL when it carries neither a key nor a ufid.
Datapath set treats a missing user-features attribute as zero, so it
clears whatever was set before. Both are in the operation doc.

The new, del and set handlers fill a reply, but they send it with
ovs_notify() to the multicast group. The caller gets an ack, not those
attributes, so the specs do not list a reply for them. get does, and
it returns the attributes to the caller.

Checked against a live kernel: flow set and delete, a delete with no
key that flushes the data path, a datapath set of the masks cache size
that also clears user features, and a vport set of the upcall pid. A
vport set that changes the type returns -EINVAL, as the handler does.

Signed-off-by: Minxi Hou <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     | 31 +++++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 15 ++++++++++
 3 files changed, 64 insertions(+)

base-commit: a5e7d8e446af9803e37a3b6a4d416fb41178348f
-- 
2.55.0

[PATCH net-next 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 | 31 +++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 951837b72e1d..87984bf1fac4 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -988,6 +988,7 @@ operations:
       doc: Create OVS flow configuration in a data path
       value: 1
       attribute-set: flow-attrs
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -995,6 +996,36 @@ operations:
             - 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
+    -
+      name: set
+      doc: |
+        Modify an existing flow. The kernel rejects the request with
+        -EINVAL when it carries neither a key nor a ufid.
+      value: 4
+      attribute-set: flow-attrs
+      flags: [uns-admin-perm]
+      do:
+        request:
+          attributes:
+            - key
+            - ufid
+            - mask
+            - actions
+            - ufid-flags
+            - clear
+            - probe
 
 mcast-groups:
   list:
-- 
2.55.0

[PATCH net-next 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 1/2] netlink: specs: add ovs_flow del and set operations

[netdev-bot+sashiko]

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

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

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` op in Documentation/netlink/specs/ovs_flow.yaml
  lists `probe` as a request attribute.
- [Low] The new `del` op in Documentation/netlink/specs/ovs_flow.yaml
  lists only key, ufid and ufid-flags.
- [Low] The new `del` and `set` ops in ovs_flow.yaml declare only
  `do.request`: no `reply`, and no op with `notify: del`/`notify: set`.
- [Low] The `set` op doc says: "The kernel rejects the request with
  -EINVAL when it carries neither a key nor a ufid." Together with the…

--

netlink: specs: add ovs_flow del and set operations

This patch adds del and set operations to the ovs_flow YNL spec. It also
marks new, del and set with uns-admin-perm. It lists the request
attributes each command reads.

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

[ ... ]

> @@ -995,6 +996,36 @@ operations:
>              - 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

[Severity: Low]
Should probe be listed here too? ovs_flow_cmd_del() reads it:

net/openvswitch/datapath.c:ovs_flow_cmd_del() {
	...
	bool log = !a[OVS_FLOW_ATTR_PROBE];
	bool ufid_present;

	ufid_present = ovs_nla_get_ufid(&ufid, a[OVS_FLOW_ATTR_UFID], log);
	...
}

log is then passed to ovs_nla_get_ufid() and ovs_nla_get_match().

The commit message says "Name the attributes each command reads", and the
set op in this patch does list probe.

C YNL only generates setters for request attributes that are listed. As a
result, a C client cannot mark a delete as a feature probe to suppress
the kernel's error logging.

[Severity: Low]
Neither del nor set describes a reply or a notification. Does the kernel
send messages for these commands that the spec doesn't cover?

ovs_flow_cmd_del() uses ovs_flow_cmd_fill_info() to build an
OVS_FLOW_CMD_DEL message. It carries the deleted flow's key, ufid, mask,
stats, actions, used and tcp-flags.

ovs_flow_cmd_set() builds an OVS_FLOW_CMD_SET message with
ovs_flow_cmd_build_info(). Both handlers send through ovs_notify(), gated
by ovs_must_notify():

net/openvswitch/datapath.c:ovs_must_notify() {
	return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
	       genl_has_listeners(family, genl_info_net(info), group);
}

A client may set NLM_F_ECHO to get the final stats of a deleted flow, or
subscribe to the ovs_flow mcast group. In both cases the spec gives it
nothing to decode these messages with.

The existing new op and the ovs_datapath and ovs_vport specs have the
same gap, so this follows existing practice in the OVS specs. Still, the
new ops describe less than the kernel sends. Could a reply or notify
description be added for del and set?

> +    -
> +      name: set
> +      doc: |
> +        Modify an existing flow. The kernel rejects the request with
> +        -EINVAL when it carries neither a key nor a ufid.

[Severity: Low]
Is this doc complete? The request list below includes both ufid and
actions, so the doc reads as though a ufid alone is enough for any set.

After the key/ufid check, ovs_flow_cmd_set() calls
ovs_nla_init_match_and_action(). That function rejects actions without a
key, even when a ufid is present:

net/openvswitch/datapath.c:ovs_nla_init_match_and_action() {
	...
	if (a[OVS_FLOW_ATTR_ACTIONS]) {
		if (!a[OVS_FLOW_ATTR_KEY]) {
			OVS_NLERR(log,
				  "Flow key attribute not present in set flow.");
			error = -EINVAL;
			goto error;
		}
	...
}

A set that carries only a ufid works for stats or clear updates. One that
replaces actions always gets -EINVAL. Could the doc say that a key is
required when actions are supplied?

> +      value: 4
> +      attribute-set: flow-attrs
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - key
> +            - ufid
> +            - mask
> +            - actions
> +            - ufid-flags
> +            - clear
> +            - probe

[Severity: Medium]
Should probe be declared as type: flag in flow-attrs? The attribute set
currently has:

        name: probe
        type: binary

The kernel policy treats it as a flag:

net/openvswitch/datapath.c:flow_policy[] = {
	...
	[OVS_FLOW_ATTR_CLEAR] = { .type = NLA_FLAG },
	[OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },
	...
}

validate_nla() in lib/nlattr.c rejects an NLA_FLAG attribute that carries
any payload:

	case NLA_FLAG:
		if (attrlen > 0)
			goto out_err;

Before this patch no op listed probe in a request, so the mismatch was not
visible. Now that set lists it, C YNL codegen will emit a pointer plus
length binary setter for probe. Any non-empty payload makes the whole
OVS_FLOW_CMD_SET fail policy validation with -EINVAL.

In Python YNL, passing "probe": true goes to the binary branch of
_add_attr() in tools/net/ynl/pyynl/lib/ynl.py. That branch raises
YnlException('Unknown type for binary attribute ...'), while the flag
branch would accept the value.

Could probe be changed to type: flag, the same as clear?

>  
>  mcast-groups:
>    list:

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

Re: [PATCH net-next 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 vport `set` op in
  Documentation/netlink/specs/ovs_vport.yaml lists only name, type,…
- [Low] The new vport `set` op lists `options` as a request attribute,
  but ovs_vport_cmd_set() returns -EOPNOTSUPP whenever…
- [Low] The new datapath `set` doc says only that a missing user-features
  attribute is treated as zero and clears the features.

--

netlink: specs: add ovs datapath and vport set operations

This adds the set operation, which the kernel already registers, to the
ovs_datapath and ovs_vport YNL specs. It also marks new, del and set in
both specs 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 how per-cpu-pids depends on user-features?

In ovs_dp_change() the new features are assigned unconditionally. The
pids are applied only when that same request turns on per-cpu dispatch:

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]);
    ...
}

Take a set request with name and per-cpu-pids where user-features is
missing or lacks OVS_DP_F_DISPATCH_UPCALL_PER_CPU, for example because
per-cpu dispatch was already on. The kernel clears or changes the
features, ignores the pids without an error, and returns success.

The spec lists per-cpu-pids as if it could be set on its own. The doc
covers only the user-features half of this coupling.

> 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 the name or the port number plus the datapath ifindex
from the 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);
    ...
}

The UAPI comment for enum ovs_vport_attr in
include/uapi/linux/openvswitch.h also says that for requests other than
new, "dp_idx from the &struct ovs_header plus %OVS_VPORT_ATTR_PORT_NO
determine the vport". The del op in this same spec already lists
port-no.

YNL code generation builds the request struct from this list. Without
port-no, a generated client can only modify a vport by name, not by
datapath ifindex plus port number. Does that only partly fix the
problem the commit message describes, that "a YNL client could not
modify an existing datapath or vport"?

> +            - type
> +            - options

[Severity: Low]
Is it useful to list options here? ovs_vport_cmd_set() rejects it every
time:

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;
	}
    ...
}

The vport_policy entry is also marked "/* Unused. */".

The commit message says the kernel "rejects options", but the op doc
only says "Modify an existing OVS vport". Generated clients therefore
get a setter for an attribute that always fails with -EOPNOTSUPP.

Could options be dropped from the set request? Or could the doc say
that it is always rejected, and that a type different from the
vport's current type gets -EINVAL?

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

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

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

[Minxi Hou]

On Thu, 8 Oct 2026, netdev-bot+sashiko@kernel.org wrote:
> [Medium] Should probe be declared as type: flag in flow-attrs?

Yes. The kernel policy marks both probe and clear as NLA_FLAG, and clear
was already a flag in the spec. probe stayed binary only because no
request listed it, so nothing exercised the mismatch. v2 makes it a flag.

> [Low] Should probe be listed on del as well? ovs_flow_cmd_del() reads it.

Yes, and v2 adds it. ovs_flow_cmd_del() reads OVS_FLOW_ATTR_PROBE to
decide whether to log the error. set listed it and del did not.

> [Low] Is the set doc complete? Replacing actions needs a key even
> when a ufid is present.

The doc was wrong. ovs_nla_init_match_and_action() returns -EINVAL for
actions with no key, whether or not a ufid is there. A ufid alone is
enough for a stats or clear update. v2 says so.

> [Low] Neither del nor set describes a reply or a notification.

Both handlers build a message and send it with ovs_notify(), and
ovs_must_notify() returns true when the request sets NLM_F_ECHO or
the multicast group has a listener. A client that wants the message
subscribes to the group.

A do() call does not get those attributes back. I checked on a live
kernel: new, set and del all return an ack, and only get returns the
flow attributes. The existing new op and the datapath and vport specs
describe the same thing the same way, which the review also notes.

Listing a reply would tell a generated client that do() returns
attributes it never receives. I left it off.

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

[Minxi Hou]

On Thu, 8 Oct 2026, netdev-bot+sashiko@kernel.org wrote:
> [Medium] Should port-no be in the vport set request list as well?

No. lookup_vport() does accept OVS_VPORT_ATTR_PORT_NO plus the
datapath ifindex from the header, and the del op in this spec lists
port-no. But get lists only name, and new does not list port-no
either. Adding it to set alone would make the three lookup keys
disagree.

The lookup key is a property of the whole spec, not of the set op
this patch adds. I left it as the file already writes it.

> [Low] Is it useful to list options here? ovs_vport_cmd_set() returns
> -EOPNOTSUPP whenever it is present.

The list is the attributes the handler reads, not the ones it
accepts. type is in the same list and a changed type returns -EINVAL
(datapath.c, ovs_vport_cmd_set()). options returns -EOPNOTSUPP and the
policy marks it unused. Dropping one and keeping the other would need
a rule for which rejections to hide. The commit message already says
the handler rejects options.

> [Low] Should the datapath set doc also say how per-cpu-pids depends
> on user-features?

ovs_dp_change() applies per-cpu-pids only when the same request turns
on OVS_DP_F_DISPATCH_UPCALL_PER_CPU. That part of the reading is
right. The doc already says that a missing user-features attribute is
taken as zero and clears the features, which is the half that changes
state. The pids are ignored and the request still succeeds. I left
that out so the doc stays shorter than the handler.