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

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

[Minxi Hou]

The ovs specs describe get and new but not the set and del commands the
kernel has registered since the families were added. This series adds
them and names the attributes each handler reads. Nothing here changes
the kernel.

v3:
  - no change since v2 beyond the items listed above.

v2:
https://lore.kernel.org/netdev/20261012065552.2426415-1-houminxi@gmail.com/
  - ovs_flow new lists ufid-flags and probe, which ovs_flow_cmd_new()
    reads.
  - The commit message now says probe changes from binary to flag, and
    that new needs CAP_NET_ADMIN.
  - The key attribute doc no longer says a key is required for every
    request except a dump. get, set and del accept a ufid with no key,
    and only a new flow needs a key. A set that replaces the actions
    still needs a key.

v1:
https://lore.kernel.org/netdev/20261006130533.1620706-1-houminxi@gmail.com/
  - probe is a flag, matching the NLA_FLAG policy. A false flag is left
    out, where the binary attribute was sent empty.
  - del lists probe.
  - The set doc says replacing the actions needs a key even when a ufid
    is present.

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     | 41 ++++++++++++++++++-
 Documentation/netlink/specs/ovs_vport.yaml    | 15 +++++++
 3 files changed, 72 insertions(+), 2 deletions(-)

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

base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
-- 
2.56.0

[PATCH net-next v3 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. new reads ufid-flags and probe
as well as the key, so both are in its request list. new, del and set
all need CAP_NET_ADMIN.

probe was binary in the spec while the kernel policy is NLA_FLAG. It
is a flag now, which changes how a YNL client encodes it: a false flag
is left out, where a binary attribute was sent empty.

The key attribute doc said a key is required for every request except
a dump. The kernel accepts a ufid with no key for get, set and del, and
only a new flow needs a key. A set that replaces the actions needs a
key too. The doc says both now.

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

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 951837b72e1d..e5e110b6ccc8 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -384,7 +384,9 @@ attribute-sets:
         nested-attributes: key-attrs
         doc: |
           Nested attributes specifying the flow key. Always present in
-          notifications. Required for all requests (except dumps).
+          notifications. A request may carry a ufid instead, except for a
+          new flow, which needs a key. A set that replaces the actions
+          needs a key too.
       -
         name: actions
         type: nest
@@ -438,7 +440,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 +990,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 +998,40 @@ operations:
             - ufid
             - mask
             - actions
+            - ufid-flags
+            - probe
+    -
+      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:
+            - key
+            - ufid
+            - mask
+            - actions
+            - ufid-flags
+            - clear
+            - probe
 
 mcast-groups:
   list:
-- 
2.56.0

[PATCH net-next v3 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.

In the kernel, the datapath SET command changes the user features, the
masks cache size and the per-cpu upcall pids. The vport SET command
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.56.0

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netlink: specs: add ovs_flow del and set operations

This patch adds the del and set operations to the ovs_flow YNL spec. It
marks new, del and set as needing admin permission and lists the
attributes each command reads. It also changes probe from binary to flag
to match the kernel policy, and rewrites the doc for the key attribute.

> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 951837b72e1d..e5e110b6ccc8 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -384,7 +384,9 @@ attribute-sets:
>          nested-attributes: key-attrs
>          doc: |
>            Nested attributes specifying the flow key. Always present in
> -          notifications. Required for all requests (except dumps).
> +          notifications. A request may carry a ufid instead, except for a
> +          new flow, which needs a key. A set that replaces the actions
> +          needs a key too.

[Severity: Low]
Should include/uapi/linux/openvswitch.h get the same update? The
kernel-doc for enum ovs_flow_attr still says:

 * @OVS_FLOW_ATTR_KEY: Nested %OVS_KEY_ATTR_* attributes specifying the flow
 * key.  Always present in notifications.  Required for all requests (except
 * dumps).

The spec text was first copied from that header. After this patch the two
in-tree descriptions of this attribute disagree, and the header still
describes a rule the kernel does not enforce.

ovs_flow_cmd_set() only rejects the request when both the key and the ufid
are missing:

	if (!a[OVS_FLOW_ATTR_KEY] && !ufid_present) {
		OVS_NLERR(log,
			  "Flow set message rejected, Key attribute missing.");
		return -EINVAL;
	}

ovs_flow_cmd_get() does the same:

	} else if (!ufid_present) {
		OVS_NLERR(log,
			  "Flow get message rejected, Key attribute missing.");
		err = -EINVAL;
	}

The new wording also drops the "except dumps" exception. "A request may
carry a ufid instead" suggests that every request other than new needs
either a key or a ufid. But ovs_flow_cmd_dump() reads neither, and a del
that carries neither is a valid flush of the data path.

Could the doc mention those two cases as well?

[Severity: Low]
This is a pre-existing issue, not one this patch introduced, but is
"Always present in notifications" accurate? This patch keeps the sentence
in the paragraph it rewrites, and the same text is in
include/uapi/linux/openvswitch.h.

ovs_flow_cmd_fill_info() calls ovs_nla_put_identifier(). For a
ufid-identified flow, that emits OVS_FLOW_ATTR_UFID instead of the key:

	if (ovs_identifier_is_ufid(&flow->id))
		return nla_put(skb, OVS_FLOW_ATTR_UFID, flow->id.ufid_len,
			       flow->id.ufid);

The key is then added only if should_fill_key() returns true:

	return ovs_identifier_is_ufid(sfid) &&
	       !(ufid_flags & OVS_UFID_F_OMIT_KEY);

So when the flow is ufid-identified and the request sets
OVS_UFID_F_OMIT_KEY in ufid-flags, the reply or notification has no key.

[ ... ]

> @@ -438,7 +440,7 @@ attribute-sets:
>            are exact match bits.
>        -
>          name: probe
> -        type: binary
> +        type: flag

[Severity: Low]
This corrects an existing mismatch with the kernel policy in flow_policy:

	[OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },

The binary type came in with the commit that added ovs_flow.yaml. Should
this change carry a tag like:

Fixes: 93b230b549bc ("netlink: specs: add ynl spec for ovs_flow")

Could it also go in a separate patch, so it can be picked up or backported
apart from the new del and set operations?

>          doc: |
>            Flow operation is a feature probe, error logging should be suppressed.

[ ... ]
-- 
pw-bot: cr

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netlink: specs: add ovs datapath and vport set operations

This adds the SET operation (cmd value 4) to the ovs_datapath and
ovs_vport YNL specs, so YNL clients can modify an existing datapath or
vport. It also adds the uns-admin-perm flag to the new, del and set
operations of both families, matching GENL_UNS_ADMIN_PERM in the kernel.

> 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

[ ... ]

> @@ -149,10 +150,27 @@ operations:

[ ... ]

> +    -
> +      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 this doc also say that per-cpu-pids only takes effect when the
same request sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU in user-features?

From ovs_dp_change() in net/openvswitch/datapath.c:

	u32 user_features = 0, old_features = dp->user_features;
	...
	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]);

A SET request with only name and per-cpu-pids, or with user-features that
lack that flag, still succeeds. The pids are dropped without an error, and
per-CPU upcall dispatch is turned off too.

The kernel behaves this way without this patch. The new op doc describes
the related user-features quirk but not this one.

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Neither
of the new set ops describes the notification the kernel sends.

ovs_dp_cmd_set() and ovs_vport_cmd_set() both build a full object message
with cmd OVS_DP_CMD_SET / OVS_VPORT_CMD_SET and multicast it:

net/openvswitch/datapath.c:ovs_vport_cmd_set() {
    ...
	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
				      info->snd_portid, info->snd_seq, 0,
				      OVS_VPORT_CMD_SET, GFP_KERNEL);
	BUG_ON(err < 0);

	ovs_unlock();
	ovs_notify(&dp_vport_genl_family, reply, info);
    ...
}

The set ops in the specs have only a do request, with no reply and no
notify. C codegen therefore generates no parser for these cmd 4 messages.
The existing new and del ops in both specs have the same gap, although
their handlers also send notifications.

The patch does help Python YNL here. handle_ntf() looks up the op with
rsp_by_value[cmd], so cmd 4 notifications can now be decoded with the op's
attribute set.

Could these notifications be described in the specs, either here or in a
follow-up?

> 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

[ ... ]

> @@ -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: Low]
Should port-no be listed here too?

ovs_vport_cmd_set() finds its target with lookup_vport(). That function
accepts either OVS_VPORT_ATTR_NAME or OVS_VPORT_ATTR_PORT_NO, using the
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);
		...
		vport = ovs_vport_ovsl_rcu(dp, port_no);
    ...
}

The del op in this spec already lists port-no. YNL C codegen only
generates setters for the attributes an op lists. So the generated
ovs_vport_set_req has no port_no setter, and a C client can only pick the
vport by name. Python YNL is not affected because it encodes from the
whole attribute set.

> +            - type
> +            - options

[Severity: Low]
Is options meant to be listed for set? ovs_vport_cmd_set() rejects every
request that carries it:

	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. */". Any
request built with the generated options setters for this op will fail
with EOPNOTSUPP.

type is similar. The kernel only checks it for equality and cannot change
it:

	if (a[OVS_VPORT_ATTR_TYPE] &&
	    nla_get_u32(a[OVS_VPORT_ATTR_TYPE]) != vport->ops->type) {
		err = -EINVAL;

The commit message says "The kernel rejects a type change and rejects
options". The spec, with its doc "Modify an existing OVS vport", says
neither.

Could options be dropped from this request, or could the doc mention
these limits?