[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

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

[Minxi Hou]

On Tue, 13 Oct 2026, netdev-bot+sashiko@kernel.org wrote:
> [Medium] The new operation's request list still leaves out ufid-flags
> and probe. ovs_flow_cmd_new() reads both.

Yes. ovs_flow_cmd_new() reads OVS_FLOW_ATTR_UFID_FLAGS and
OVS_FLOW_ATTR_PROBE, and the del and set lists in this patch already
have them. The new list did not. v3 adds both.

> [Low] The commit message does not mention the probe type change or the
> uns-admin-perm flag added to the existing new operation.

Right, it only said del and set need CAP_NET_ADMIN. new takes the same
flag, and probe changes from binary to flag, which changes how a YNL
client encodes it. v3 says both.

> [Low] The key attribute doc says a key is required for every request
> except a dump, which contradicts the new del and set docs.

The doc was wrong. get, set and del all accept a ufid with no key, and
only a new flow needs one. A set that replaces the actions still needs
a key, which the set doc already says. v3 changes the key doc.

> [Low] The get operation uses one request list for both do and dump.

That list predates this series. get's do request reads probe and the
dump request only reads ufid-flags, so the two should not share a list.
I am leaving it for a separate patch rather than widening this one.

pw-bot: cr

[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

[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

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

[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 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 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.