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

[PATCH net-next v7 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
how the kernel behaves.

v7:
https://lore.kernel.org/netdev/20261107120647.874906-1@gmail.com/
  - The ufid docs said a value outside 1-16 octets is read as absent, and
    that a dump ignores the attribute. Only a value longer than 16 octets
    is read as absent. A zero length value fails the policy check before
    the command runs, and the request fails with -ERANGE; a dump accepts
    the attribute and does not read it.
  - The del and key docs said that a del with an out of range ufid
    flushes the data path. That only holds when the request carries no
    key, which the text now says.
  - The get reply list names the last-used time and the TCP flags, which
    ovs_flow_cmd_fill_stats() fills for get and for dump, and the docs of
    the stats, the used time and the TCP flags now say replies and
    notifications rather than notifications only.
  - The vport set doc says what the command does instead of explaining
    why two attributes are missing from the list, and says that the type
    only has to match.
  - The datapath new doc reads as a sentence again, the masks cache size
    records its upper bound of 4096, and the vport new request list gains
    the port number, which its handler reads.
  - Patch 2 says why ifindex is s32, which is to match the policy and
    nla_get_s32_default() in the request path, and that the generated
    reply struct follows with a __s32 field while the reply itself keeps
    putting the value as u32.

v6:
https://lore.kernel.org/netdev/20261103183319.2730567-1-houminxi@gmail.com/
  - A del flushes the data path when it carries no key and no valid ufid,
    not when it carries neither. The kernel reads a ufid outside 1-16
    octets as absent, so such a request flushes as well. The ufid doc now
    says that, and that a new request with such a value indexes the flow
    by its key instead, and the attribute records the minimum length the
    policy enforces. The same sentences in
    include/uapi/linux/openvswitch.h follow.
  - ifindex is declared s32 with a minimum of 0, which is how both
    policies read it. No request used the attribute before this series,
    which is why the type went unnoticed.
  - The ops skip strict and dump validation in the kernel, so get, new,
    del and set say so in all three specs.
  - Rebased onto current net-next.

v5:
https://lore.kernel.org/netdev/20261025103409.3519874-1-houminxi@gmail.com/
  - ovs_flow dump gets a request list of its own with ufid-flags. It
    shared the get list, which offered a key and a ufid the kernel
    ignores on a dump. The reply is unchanged.
  - The key, actions and ufid docs no longer claim those attributes are
    always present, and say what the exceptions are: a ufid-identified
    flow sends no key or actions when the request asked for them to be
    left out, a dump leaves the actions out when they do not fit, and a
    dump ignores the ufid. The same sentences in
    include/uapi/linux/openvswitch.h are updated to match, since the
    spec text was copied from there.
  - The key doc now says a get and a set need a key or a ufid, that a
    new request needs a key, and that a del carrying neither flushes the
    data path.
  - The ovs_datapath new and set docs say per-cpu-pids takes effect only
    when the same request sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU in
    user-features.
  - The ovs_vport set doc says the vport is picked by name or by port
    number, names the rejections in the order the kernel checks them,
    and says upcall-pid is the only attribute that changes state.
  - Rebased onto current net-next after the merge window.

v4:
https://lore.kernel.org/netdev/20261015024132.698658-1-houminxi@gmail.com/
  - ovs_vport set lists port-no and ifindex. lookup_vport() finds a
    vport by name or by port number, and returns -EOPNOTSUPP as soon as
    it sees an ifindex, before the type or the options are looked at.
  - ovs_datapath new lists masks-cache-size, per-cpu-pids and ifindex.
    new calls ovs_dp_change(), which reads the first two, and reads
    ifindex itself.
  - ovs_flow get lists probe. ovs_flow_cmd_get() reads it to decide
    whether to log.
  - The key attribute doc now says a new request needs a key even when
    it finds an existing flow and replaces its actions, and that a dump
    reads neither the key nor the ufid. A del with neither flushes the
    data path, and the del operation doc says so too.

v3:
https://lore.kernel.org/netdev/20261012065552.2426415-1-houminxi@gmail.com/
  - no change since v2.

v2:
https://lore.kernel.org/netdev/20261006130533.1620706-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.

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

Minxi Hou (2):
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 |  36 ++++++-
 Documentation/netlink/specs/ovs_flow.yaml     | 102 ++++++++++++++----
 Documentation/netlink/specs/ovs_vport.yaml    |  29 ++++-
 include/uapi/linux/openvswitch.h              |  35 +++---
 4 files changed, 167 insertions(+), 35 deletions(-)


base-commit: fa5196392691051963726787944db872420dfa75
-- 
2.56.0

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

[Minxi Hou]

The ovs_flow spec described get and new only, so a YNL client could not
delete or modify a flow. Both commands have been in the kernel since the
family was added: del flushes the data path when a request carries
neither a key nor a ufid, or no key and a ufid longer than 16 octets, and
set looks a flow up by key or by ufid.

List the attributes each command reads. new reads ufid-flags and probe as
well as the key, so those go in its request list. new, del and set need
CAP_NET_ADMIN.

A dump reads ufid-flags and nothing else. It shared the get request list,
which let a client send a key and a ufid the kernel ignores, so give it a
request list of its own. The reply stays the same as for get.

probe was binary while the kernel policy is NLA_FLAG, so a client that
wanted to raise it had to send an attribute with no body. As a flag the
generated setter takes no value.

The key and the actions docs called those attributes always present in
notifications. A flow identified by a ufid sends neither when the request
asked for it to be left out, and a get reply follows the same rule. A
dump also leaves the actions out when they do not fit in the message.

The key doc said a key is required for every request except a dump. The
kernel accepts a ufid with no key, but a get and a set need one of the
two, and a set that replaces the actions needs a key. A new request needs
a key, including one that finds an existing flow and replaces its
actions.

The ufid doc promised that the flow is indexed by that value, which does
not hold for a value the kernel rejects. A value longer than 16 octets is
read as if the attribute were absent, so a new request indexes the flow
by its key instead, and a del that carries no key flushes the data path.
A zero length value fails the policy check before the command runs and
the request fails with -ERANGE. A dump does not read the attribute at
all.

The get reply carries the last-used time, the stats and the TCP flags,
which ovs_flow_cmd_fill_stats() fills for get and for dump. The reply
list named only the stats, so a generated client parsed neither of the
other two, and their own docs called them notification-only. List them
and say where they appear.

Fix the same sentences in the uapi header: the spec text was copied from
there, and the two descriptions disagreed once this patch changed the
spec.

The ops skip strict and dump validation in the kernel, so the spec says
so for get, new, del and set.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 Documentation/netlink/specs/ovs_flow.yaml | 102 +++++++++++++++++-----
 include/uapi/linux/openvswitch.h          |  35 +++++---
 2 files changed, 105 insertions(+), 32 deletions(-)

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 951837b72e1d..d7aeeeb5a3cc 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -383,41 +383,50 @@ attribute-sets:
         type: nest
         nested-attributes: key-attrs
         doc: |
-          Nested attributes specifying the flow key. Always present in
-          notifications. Required for all requests (except dumps).
+          Nested attributes specifying the flow key. Present in replies and
+          notifications, except for a flow identified by a ufid when the
+          request asked for the key to be left out. A get, set or del may
+          carry a ufid instead of a key; a get and a set need one of the
+          two, and a set that replaces the actions needs a key. A new
+          request needs a key, including one that finds an existing flow and
+          replaces its actions. A dump reads neither the key nor the ufid. A
+          del that carries neither a key nor a ufid flushes the data path, and
+          so does one that carries no key and a ufid longer than 16 octets.
       -
         name: actions
         type: nest
         nested-attributes: action-attrs
         doc: |
           Nested attributes specifying the actions to take for packets that
-          match the key. Always present in notifications. Required for
-          OVS_FLOW_CMD_NEW requests, optional for OVS_FLOW_CMD_SET requests.  An
-          OVS_FLOW_CMD_SET without OVS_FLOW_ATTR_ACTIONS will not modify the
-          actions.  To clear the actions, an OVS_FLOW_ATTR_ACTIONS without any
-          nested attributes must be given.
+          match the key. Present in replies and notifications, except when
+          the request asked for the actions to be left out, and in a dump
+          when they do not fit in the message. Required for
+          OVS_FLOW_CMD_NEW requests, optional for OVS_FLOW_CMD_SET requests.
+          An OVS_FLOW_CMD_SET without OVS_FLOW_ATTR_ACTIONS will not modify
+          the actions. To clear the actions, an OVS_FLOW_ATTR_ACTIONS
+          without any nested attributes must be given.
       -
         name: stats
         type: binary
         struct: ovs-flow-stats
         doc: |
-          Statistics for this flow. Present in notifications if the stats would
-          be nonzero. Ignored in requests.
+          Statistics for this flow. Present in replies and notifications if
+          the stats would be nonzero. Ignored in requests.
       -
         name: tcp-flags
         type: u8
         doc: |
           An 8-bit value giving the ORed value of all of the TCP flags seen on
-          packets in this flow. Only present in notifications for TCP flows, and
-          only if it would be nonzero. Ignored in requests.
+          packets in this flow. Only present in replies and notifications for
+          TCP flows, and only if it would be nonzero. Ignored in requests.
       -
         name: used
         type: u64
         doc: |
           A 64-bit integer giving the time, in milliseconds on the system
           monotonic clock, at which a packet was last processed for this
-          flow. Only present in notifications if a packet has been processed for
-          this flow. Ignored in requests.
+          flow. Only present in replies and notifications if a packet has been
+          processed for this flow. Ignored in requests.
       -
         name: clear
         type: flag
@@ -438,18 +447,22 @@ attribute-sets:
           are exact match bits.
       -
         name: probe
-        type: binary
+        type: flag
         doc: |
           Flow operation is a feature probe, error logging should be suppressed.
       -
         name: ufid
         type: binary
+        checks:
+          min-len: 1
         doc: |
           A value between 1-16 octets specifying a unique identifier for the
           flow. Causes the flow to be indexed by this value rather than the
-          value of the OVS_FLOW_ATTR_KEY attribute. Optional for all
-          requests. Present in notifications if the flow was created with this
-          attribute.
+          value of the OVS_FLOW_ATTR_KEY attribute. A value longer than 16
+          octets is treated as if the attribute were absent, so a new flow is
+          indexed by its key. A zero-length value is rejected with -ERANGE.
+          Optional for all requests. Present in notifications if the flow was
+          created with this attribute. A dump does not read it.
         display-hint: uuid
       -
         name: ufid-flags
@@ -969,25 +982,35 @@ operations:
       doc: Get / dump OVS flow configuration and state
       value: 3
       attribute-set: flow-attrs
-      do: &flow-get-op
+      dont-validate: [strict, dump]
+      do:
         request:
           attributes:
             - key
             - ufid
             - ufid-flags
-        reply:
+            - probe
+        reply: &flow-get-reply
           attributes:
             - key
             - ufid
             - mask
+            - used
             - stats
+            - tcp-flags
             - actions
-      dump: *flow-get-op
+      dump:
+        request:
+          attributes:
+            - ufid-flags
+        reply: *flow-get-reply
     -
       name: new
       doc: Create OVS flow configuration in a data path
       value: 1
       attribute-set: flow-attrs
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -995,6 +1018,45 @@ operations:
             - ufid
             - mask
             - actions
+            - ufid-flags
+            - probe
+    -
+      name: del
+      doc: |
+        Delete one flow, or every flow in the data path. A request that
+        carries neither a key nor a ufid flushes the data path, and so does
+        one that carries no key and a ufid longer than 16 octets.
+      value: 2
+      attribute-set: flow-attrs
+      dont-validate: [strict, dump]
+      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
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
+      do:
+        request:
+          attributes:
+            - key
+            - ufid
+            - mask
+            - actions
+            - ufid-flags
+            - clear
+            - probe
 
 mcast-groups:
   list:
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 440825e65837..3f18f59e91dc 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -624,24 +624,33 @@ struct ovs_nsh_key_md1 {
 /**
  * enum ovs_flow_attr - attributes for %OVS_FLOW_* commands.
  * @OVS_FLOW_ATTR_KEY: Nested %OVS_KEY_ATTR_* attributes specifying the flow
- * key.  Always present in notifications.  Required for all requests (except
- * dumps).
+ * key.  Present in replies and notifications, except for a flow identified
+ * by a ufid when the request asked for the key to be left out.  A get, set
+ * or del may carry a ufid instead of a key; a get and a set need one of the
+ * two, and a set that replaces the actions needs a key.  A new request needs
+ * a key, including one that finds an existing flow and replaces its
+ * actions.  A dump reads neither of the two, and a del carrying neither a
+ * key nor a ufid flushes the data path, as does one carrying no key and a
+ * ufid longer than 16 octets.
  * @OVS_FLOW_ATTR_ACTIONS: Nested %OVS_ACTION_ATTR_* attributes specifying
- * the actions to take for packets that match the key.  Always present in
- * notifications.  Required for %OVS_FLOW_CMD_NEW requests, optional for
+ * the actions to take for packets that match the key.  Present in replies
+ * and notifications, except when the request asked for the actions to be
+ * left out, and in a dump when they do not fit in the message.  Required
+ * for %OVS_FLOW_CMD_NEW requests, optional for
  * %OVS_FLOW_CMD_SET requests.  An %OVS_FLOW_CMD_SET without
  * %OVS_FLOW_ATTR_ACTIONS will not modify the actions.  To clear the actions,
  * an %OVS_FLOW_ATTR_ACTIONS without any nested attributes must be given.
  * @OVS_FLOW_ATTR_STATS: &struct ovs_flow_stats giving statistics for this
- * flow.  Present in notifications if the stats would be nonzero.  Ignored in
- * requests.
+ * flow.  Present in replies and notifications if the stats would be nonzero.
+ * Ignored in requests.
  * @OVS_FLOW_ATTR_TCP_FLAGS: An 8-bit value giving the OR'd value of all of the
- * TCP flags seen on packets in this flow.  Only present in notifications for
- * TCP flows, and only if it would be nonzero.  Ignored in requests.
+ * TCP flags seen on packets in this flow.  Only present in replies and
+ * notifications for TCP flows, and only if it would be nonzero.  Ignored in
+ * requests.
  * @OVS_FLOW_ATTR_USED: A 64-bit integer giving the time, in milliseconds on
  * the system monotonic clock, at which a packet was last processed for this
- * flow.  Only present in notifications if a packet has been processed for this
- * flow.  Ignored in requests.
+ * flow.  Only present in replies and notifications if a packet has been
+ * processed for this flow.  Ignored in requests.
  * @OVS_FLOW_ATTR_CLEAR: If present in a %OVS_FLOW_CMD_SET request, clears the
  * last-used time, accumulated TCP flags, and statistics for this flow.
  * Otherwise ignored in requests.  Never present in notifications.
@@ -655,9 +664,11 @@ struct ovs_nsh_key_md1 {
  * should be suppressed.
  * @OVS_FLOW_ATTR_UFID: A value between 1-16 octets specifying a unique
  * identifier for the flow. Causes the flow to be indexed by this value rather
- * than the value of the %OVS_FLOW_ATTR_KEY attribute. Optional for all
+ * than the value of the %OVS_FLOW_ATTR_KEY attribute. A value longer than 16
+ * octets is treated as if the attribute were absent, so a new flow is indexed
+ * by its key. A zero-length value is rejected with -ERANGE. Optional for all
  * requests. Present in notifications if the flow was created with this
- * attribute.
+ * attribute. A dump does not read it.
  * @OVS_FLOW_ATTR_UFID_FLAGS: A 32-bit value of OR'd %OVS_UFID_F_*
  * flags that provide alternative semantics for flow installation and
  * retrieval. Optional for all requests.
-- 
2.56.0

[PATCH net-next v7 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 change an existing datapath or vport. The datapath
NEW command also gains the masks cache size, the per-cpu upcall pids and
the ifindex, which its handler reads. new, del and set take
CAP_NET_ADMIN, which is 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. A request that leaves out user-features is
read as zero, which clears the features set earlier. per-cpu-pids takes
effect only when the same request sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU
in user-features; without that bit the pids are dropped and the request
still succeeds. The masks cache size has an upper bound of 4096, which
the attribute records.

The vport SET command changes the upcall pid. It finds the vport by name,
or by port number when the request carries no name. An ifindex or options
are rejected with -EOPNOTSUPP, and a type that differs from the vport's
type with -EINVAL. The type only has to match. The vport NEW request list
gains the port number, which its handler reads.

The ifindex attribute is signed in both policies, which read it with
nla_get_s32_default() and reject anything below zero, so declare it as
s32 to match them rather than leaving it u32. The field in the generated
reply struct becomes __s32 with it. The reply itself does not change:
ovs_vport_cmd_fill_info() still puts the value as u32, and the two types
have the same width.

The ops in both families skip strict and dump validation in the kernel,
so the spec says so for get, new, del and set.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 Documentation/netlink/specs/ovs_datapath.yaml | 36 +++++++++++++++++--
 Documentation/netlink/specs/ovs_vport.yaml    | 29 ++++++++++++++-
 2 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
index f7b3671991e6..b24dc1e03f46 100644
--- a/Documentation/netlink/specs/ovs_datapath.yaml
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -102,13 +102,17 @@ attribute-sets:
       -
         name: masks-cache-size
         type: u32
+        checks:
+          max: 4096
       -
         name: per-cpu-pids
         type: binary
         sub-type: u32
       -
         name: ifindex
-        type: u32
+        type: s32
+        checks:
+          min: 0
 
 operations:
   fixed-header: ovs-header
@@ -119,6 +123,7 @@ operations:
       doc: Get / dump OVS data path configuration and state
       value: 3
       attribute-set: datapath
+      dont-validate: [strict, dump]
       do: &dp-get-op
         request:
           attributes:
@@ -135,24 +140,51 @@ operations:
       dump: *dp-get-op
     -
       name: new
-      doc: Create new OVS data path
+      doc: |
+        Create new OVS data path. per-cpu-pids takes effect only when the
+        same request sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU in user-features.
       value: 1
       attribute-set: datapath
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
             - name
             - upcall-pid
             - user-features
+            - masks-cache-size
+            - per-cpu-pids
+            - ifindex
     -
       name: del
       doc: Delete existing OVS data path
       value: 2
       attribute-set: datapath
+      dont-validate: [strict, dump]
+      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. per-cpu-pids takes effect only when the same request
+        sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU in user-features.
+      value: 4
+      attribute-set: datapath
+      dont-validate: [strict, dump]
+      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..5a3ac4b03033 100644
--- a/Documentation/netlink/specs/ovs_vport.yaml
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -111,7 +111,9 @@ attribute-sets:
         type: unused
       -
         name: ifindex
-        type: u32
+        type: s32
+        checks:
+          min: 0
       -
         name: netnsid
         type: u32
@@ -128,18 +130,23 @@ operations:
       name: new
       doc: Create a new OVS vport
       attribute-set: vport
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
             - name
             - type
             - upcall-pid
+            - port-no
             - ifindex
             - options
     -
       name: del
       doc: Delete existing OVS vport from a data path
       attribute-set: vport
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -150,6 +157,7 @@ operations:
       name: get
       doc: Get / dump OVS vport configuration and state
       attribute-set: vport
+      dont-validate: [strict, dump]
       do: &vport-get-op
         request:
           attributes:
@@ -165,6 +173,25 @@ operations:
             - netnsid
             - upcall-stats
       dump: *vport-get-op
+    -
+      name: set
+      doc: |
+        Modify an existing OVS vport. The vport is picked by name, or by
+        port number when the request carries no name. An ifindex or options
+        are rejected with -EOPNOTSUPP, and a type that differs from the
+        vport's type with -EINVAL. The type only has to match; the one
+        attribute that changes state is upcall-pid.
+      value: 4
+      attribute-set: vport
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
+      do:
+        request:
+          attributes:
+            - name
+            - type
+            - upcall-pid
+            - port-no
 
 mcast-groups:
   list:
-- 
2.56.0

Re: [PATCH net-next v7 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 new OVS_FLOW_ATTR_UFID doc in
  include/uapi/linux/openvswitch.h (lines 669-671) and in…
- [Low] The patch changes the docs for key, actions, stats, tcp-flags and
  used from 'present in notifications' to 'present in replies and…

--

netlink: specs: add ovs_flow del and set operations

This commit adds the del and set operations to the ovs_flow YNL spec and
gives dump its own request list. It also changes probe to a flag and adds
used and tcp-flags to the get reply. The attribute docs are reworded in
both the spec and include/uapi/linux/openvswitch.h.

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

[ ... ]

> @@ -438,18 +447,22 @@ attribute-sets:

[ ... ]

>        -
>          name: ufid
>          type: binary
> +        checks:
> +          min-len: 1
>          doc: |
>            A value between 1-16 octets specifying a unique identifier for the
>            flow. Causes the flow to be indexed by this value rather than the
> -          value of the OVS_FLOW_ATTR_KEY attribute. Optional for all
> -          requests. Present in notifications if the flow was created with this
> -          attribute.
> +          value of the OVS_FLOW_ATTR_KEY attribute. A value longer than 16
> +          octets is treated as if the attribute were absent, so a new flow is
> +          indexed by its key. A zero-length value is rejected with -ERANGE.
> +          Optional for all requests. Present in notifications if the flow was
> +          created with this attribute. A dump does not read it.

[Severity: Low]
Is "A dump does not read it" accurate when it sits next to the -ERANGE
sentence? The commit message goes further:

  "A dump does not read the attribute at all."

ovs_flow_cmd_dump() still parses every attribute against flow_policy:

net/openvswitch/datapath.c:ovs_flow_cmd_dump() {
    ...
	err = genlmsg_parse_deprecated(cb->nlh, &dp_flow_genl_family, a,
				       OVS_FLOW_ATTR_MAX, flow_policy, NULL);
	if (err)
		return err;
    ...
}

and flow_policy has:

	[OVS_FLOW_ATTR_UFID] = { .type = NLA_UNSPEC, .len = 1 },

In validate_nla(), the NLA_UNSPEC length check fails with -ERANGE, so a dump
request that carries a zero-length OVS_FLOW_ATTR_UFID is rejected the same
way a do request is. The dump never uses the ufid value, but it does
validate the attribute.

Could the wording say that a dump ignores the value but still rejects a
zero-length ufid? The same sentence is in the OVS_FLOW_ATTR_UFID kernel-doc
in include/uapi/linux/openvswitch.h.

[ ... ]

> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 440825e658371..3f18f59e91dc2 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h

[ ... ]

> @@ -655,9 +664,11 @@ struct ovs_nsh_key_md1 {
>   * should be suppressed.
>   * @OVS_FLOW_ATTR_UFID: A value between 1-16 octets specifying a unique
>   * identifier for the flow. Causes the flow to be indexed by this value rather
> - * than the value of the %OVS_FLOW_ATTR_KEY attribute. Optional for all
> + * than the value of the %OVS_FLOW_ATTR_KEY attribute. A value longer than 16
> + * octets is treated as if the attribute were absent, so a new flow is indexed
> + * by its key. A zero-length value is rejected with -ERANGE. Optional for all
>   * requests. Present in notifications if the flow was created with this
> - * attribute.
> + * attribute. A dump does not read it.

[Severity: Low]
This patch changes key, actions, stats, tcp-flags and used from "present in
notifications" to "present in replies and notifications". This paragraph is
edited too, but it keeps "Present in notifications if the flow was created
with this attribute". Should it also say replies?

ovs_flow_cmd_fill_info() calls ovs_nla_put_identifier() unconditionally:

net/openvswitch/datapath.c:ovs_flow_cmd_fill_info() {
    ...
	err = ovs_nla_put_identifier(flow, skb);
	if (err)
		goto error;
    ...
}

and for a flow identified by a ufid, that emits OVS_FLOW_ATTR_UFID:

net/openvswitch/flow_netlink.c:ovs_nla_put_identifier() {
	if (ovs_identifier_is_ufid(&flow->id))
		return nla_put(skb, OVS_FLOW_ATTR_UFID, flow->id.ufid_len,
			       flow->id.ufid);
    ...
}

fill_info() builds get replies and every dump entry, so the ufid shows up in
those replies as well as in notifications.

The spec now also disagrees with itself: the &flow-get-reply list includes
ufid, but the ufid doc in ovs_flow.yaml has the same notification-only
sentence.

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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 patch adds `checks: max: 4096` to `masks-cache-size` in
  Documentation/netlink/specs/ovs_datapath.yaml.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  Documentation/netlink/specs/ovs_vport.yaml still declares the vport…
- [Low] This problem wasn't introduced by this patch, but the vport `get`
  do request in Documentation/netlink/specs/ovs_vport.yaml lists only…
- [Low] The new vport and datapath `set` ops in the specs declare no
  reply and no notification.

--

netlink: specs: add ovs datapath and vport set operations

This commit adds SET operations to the ovs_datapath and ovs_vport netlink
specs and adds more attributes to the NEW request lists. It also marks
new, del and set as uns-admin-perm and changes ifindex to s32 with a
minimum of 0. Finally it puts a maximum of 4096 on masks-cache-size and
marks all ops as dont-validate strict and dump.

> diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
> index f7b3671991e6c..b24dc1e03f46a 100644
> --- a/Documentation/netlink/specs/ovs_datapath.yaml
> +++ b/Documentation/netlink/specs/ovs_datapath.yaml
> @@ -102,13 +102,17 @@ attribute-sets:
>        -
>          name: masks-cache-size
>          type: u32
> +        checks:
> +          max: 4096

[Severity: Medium]
Is 4096 the right bound on every configuration? The commit message says:

  The masks cache size has an upper bound of 4096, which the attribute
  records.

However, datapath_policy in net/openvswitch/datapath.c computes the limit
from the percpu unit size:

	[OVS_DP_ATTR_MASKS_CACHE_SIZE] =  NLA_POLICY_RANGE(NLA_U32, 0,
		PCPU_MIN_UNIT_SIZE / sizeof(struct mask_cache_entry)),

and include/linux/percpu.h has:

#define PCPU_MIN_UNIT_SIZE		PFN_ALIGN(32 << 10)

struct mask_cache_entry is 8 bytes. That gives a limit of 4096 on 4K and
16K page kernels, 8192 with 64K pages (ppc64, arm64 with
CONFIG_ARM64_64K_PAGES) and 32768 with 256K pages. On those
configurations the spec would mark values that the kernel accepts as out
of range.

The spec also leaves out a second rule.
ovs_flow_tbl_masks_cache_resize() in net/openvswitch/flow_table.c rejects
any non-zero size that is not a power of two:

	if ((!is_power_of_2(size) && size != 0) ||
	    (size * sizeof(struct mask_cache_entry)) > PCPU_MIN_UNIT_SIZE)
		return -EINVAL;

Could the max check be dropped? Another option is to document the bound
as PCPU_MIN_UNIT_SIZE / 8, along with the zero-or-power-of-two rule.

[ ... ]

> diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
> index fa975f8821b6c..5a3ac4b03033a 100644
> --- a/Documentation/netlink/specs/ovs_vport.yaml
> +++ b/Documentation/netlink/specs/ovs_vport.yaml
> @@ -111,7 +111,9 @@ attribute-sets:
>          type: unused
>        -
>          name: ifindex
> -        type: u32
> +        type: s32
> +        checks:
> +          min: 0
>        -
>          name: netnsid
>          type: u32

[Severity: Low]
This isn't a bug introduced by this patch. Since ifindex is being matched
to the kernel policy here, should netnsid be fixed the same way? The spec
still declares netnsid as u32, but vport_policy in
net/openvswitch/datapath.c has:

	[OVS_VPORT_ATTR_NETNSID] = { .type = NLA_S32 },

and ovs_vport_cmd_fill_info() sends it with nla_put_s32():

		int id = peernet2id_alloc(net, net_vport, GFP_ATOMIC);

		if (nla_put_s32(skb, OVS_VPORT_ATTR_NETNSID, id))

peernet2id_alloc() can return NETNSA_NSID_NOT_ASSIGNED (-1). A YNL client
that follows the spec would read that as 4294967295.

[ ... ]

> @@ -150,6 +157,7 @@ operations:
>        name: get
>        doc: Get / dump OVS vport configuration and state
>        attribute-set: vport
> +      dont-validate: [strict, dump]
>        do: &vport-get-op
>          request:
>            attributes:

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The get
do request still lists only name. ovs_vport_cmd_get() uses the same
lookup_vport() helper as set and del, and that helper falls back to the
port number when the request has no name:

	} else if (a[OVS_VPORT_ATTR_PORT_NO]) {
		u32 port_no = nla_get_u32(a[OVS_VPORT_ATTR_PORT_NO]);

del and the new set op both list port-no. Should get list it too, so that
clients generated from the spec can look up a vport by port number?

> @@ -165,6 +173,25 @@ operations:
>              - netnsid
>              - upcall-stats
>        dump: *vport-get-op
> +    -
> +      name: set
> +      doc: |
> +        Modify an existing OVS vport. The vport is picked by name, or by
> +        port number when the request carries no name. An ifindex or options
> +        are rejected with -EOPNOTSUPP, and a type that differs from the
> +        vport's type with -EINVAL. The type only has to match; the one
> +        attribute that changes state is upcall-pid.
> +      value: 4
> +      attribute-set: vport
> +      dont-validate: [strict, dump]
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - name
> +            - type
> +            - upcall-pid
> +            - port-no

[Severity: Low]
This is a pre-existing gap, and the existing new and del ops have it too.
The new set op here declares no reply or notification, and neither does
the datapath set op in ovs_datapath.yaml.

ovs_vport_cmd_set() builds a full vport message and sends it with
ovs_notify(), which calls genl_notify():

	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);

ovs_dp_cmd_set() does the same with OVS_DP_CMD_SET. These messages go to
the family's multicast group. They also go to the requester when
NLM_F_ECHO is set.

Should the set ops describe these messages, for example with a notify
entry that reuses the get reply, so generated clients can decode them?

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

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

[Jakub Kicinski]

On Thu, 12 Nov 2026 02:02:26 +0000 netdev-bot+sashiko@kernel.org wrote:
> > @@ -102,13 +102,17 @@ attribute-sets:
> >        -
> >          name: masks-cache-size
> >          type: u32
> > +        checks:
> > +          max: 4096  
> 
> [Severity: Medium]
> Is 4096 the right bound on every configuration? 

Just don't add the bound if it can't be easily expressed.
We are not planning to code gen the policy anyway, AFAIU

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

[Jakub Kicinski]

On Thu, 12 Nov 2026 02:02:25 +0000 netdev-bot+sashiko@kernel.org wrote:
> > +          value of the OVS_FLOW_ATTR_KEY attribute. A value longer than 16
> > +          octets is treated as if the attribute were absent, so a new flow is
> > +          indexed by its key. A zero-length value is rejected with -ERANGE.
> > +          Optional for all requests. Present in notifications if the flow was
> > +          created with this attribute. A dump does not read it.  
> 
> [Severity: Low]
> Is "A dump does not read it" accurate when it sits next to the -ERANGE
> sentence? The commit message goes further:
> 
>   "A dump does not read the attribute at all."

Please don't over-describe things if the description isn't 100%
correct.. IDK if you added this here based on previous AI feedback
but if so - please feel free to ignore the AI feedback about
documenting things. It's often wrong / leading down the wrong path.