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

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

[Minxi Hou]

The ovs specs describe get and new but not every command the kernel
has registered: all three families lacked set, and ovs_flow also lacked
del. This series adds them, names the attributes each handler reads,
and corrects what the existing specs say about the kernel. Nothing
here changes how the kernel behaves.

v8:
https://lore.kernel.org/netdev/20261110174751.973464-1@gmail.com/
  - The datapath spec no longer puts a maximum on masks-cache-size. The
    generator does not turn a check into a policy for these families, and
    the kernel bound is an expression, so the declaration promised
    something the spec cannot express.
  - The docs no longer say what a dump does with a key, a ufid or another
    request attribute. A dump now has its own request list for each family,
    which states what it takes, so the sentences that repeated it are gone.
    The ufid docs record the minimum length the policy sets, and the key,
    actions, stats, used and tcp-flags docs say the attributes appear in
    replies as well as in notifications.
  - The series gains a third patch for gaps that predate it. The vport
    netnsid is signed in the kernel. The two dumps stop sharing a request
    list with their get operation, and four lists named attributes their
    handler never reads or sends. The vport get request can be answered by
    port number, and the vport new doc says an ifindex needs an internal
    vport. The vport spec declared family version 2 where the kernel
    registers 1.
  - That patch also declares what the kernel registers: all four ovs specs
    document the header struct and none of them a member of it, the vport
    name note names the header member the lookup uses, the options note
    says what a set returns, the packet execute operation gains the
    validate flags and the admin permission, and the ynl selftest stops
    printing a field the datapath reply does not carry.
  - Deleting one flow sends that flow to the family's multicast group, a
    flush sends nothing, and the set operations do the same for the object
    they change, which those op docs now say.

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 (3):
  netlink: specs: add ovs_flow del and set operations
  netlink: specs: add ovs datapath and vport set operations
  netlink: specs: correct the ovs specs where they do not match the
    kernel

 Documentation/netlink/specs/ovs_datapath.yaml |  53 +++++++--
 Documentation/netlink/specs/ovs_flow.yaml     | 110 ++++++++++++++----
 Documentation/netlink/specs/ovs_packet.yaml   |  20 +++-
 Documentation/netlink/specs/ovs_vport.yaml    |  55 +++++++--
 include/uapi/linux/openvswitch.h              |  49 +++++---
 tools/net/ynl/tests/ovs.c                     |   5 +-
 6 files changed, 225 insertions(+), 67 deletions(-)


base-commit: 6964ff21c5d8b75649211a52dbfd92753e605649
-- 
2.56.0

[PATCH net-next v8 1/3] 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, and say that a successful delete of
one flow, or a successful set, sends that flow to the family's multicast
group, as the kernel does. new and get read ufid-flags and probe as well as
the key, so those belong in their request lists. The get reply gains the
last-used time and the TCP flags, which ovs_flow_cmd_fill_stats() fills for
a get and for a dump reply. new, del and set need CAP_NET_ADMIN, and all
four operations skip strict and dump validation.

A dump reads ufid-flags alone, so give it a request list of its own instead
of sharing the get list. 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, actions, stats, used and tcp-flags docs called those attributes
always present in notifications. The key is left out for a flow identified
by a ufid when the request asked for the key to be left out, and a dump
also leaves the actions out when they do not fit in the message. The other
three appear in replies as well, which the docs now say, and the uapi
comments for those attributes say the same.

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 header definition said a zero data path member leaves the request
non-specific, which no flow request accepts: each of them returns -ENODEV.
Drop that description.

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 minimum length the policy sets. Record that
minimum, and limit the promise about indexing to a value the kernel
accepts. The doc also said the attribute appears in notifications only,
while a reply carries it too.

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

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 951837b72e1d..98ef461535cd 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -18,9 +18,6 @@ definitions:
       -
         name: dp-ifindex
         type: u32
-        doc: |
-          ifindex of local port for datapath (0 to make a request not specific
-          to a datapath).
   -
     name: ovs-flow-stats
     type: struct
@@ -383,41 +380,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 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 +444,21 @@ 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. Optional for all requests. Present in replies
+          and notifications if the flow was created with this attribute.
         display-hint: uuid
       -
         name: ufid-flags
@@ -969,25 +978,77 @@ 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:
+            - key
+            - 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. A
+        successful delete of a single flow also sends that flow to the
+        family's multicast group, and to the requester when the request
+        sets NLM_F_ECHO; a flush sends nothing.
+      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. A
+        successful set also sends the updated flow to the family's
+        multicast group, and to the requester when the request sets
+        NLM_F_ECHO.
+      value: 4
+      attribute-set: flow-attrs
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -995,6 +1056,9 @@ operations:
             - 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..bd77e6fb4a95 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -624,24 +624,32 @@ 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 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 +663,10 @@ 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
- * requests. Present in notifications if the flow was created with this
- * attribute.
+ * 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. Optional for all requests. Present in replies and notifications
+ * if the flow was created with this attribute.
  * @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 v8 2/3] 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 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.

Both SET commands also send the updated object to the family's multicast
group, and to the requester when the request sets NLM_F_ECHO, which the
new op docs say.

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

diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
index f7b3671991e6..6232760224dc 100644
--- a/Documentation/netlink/specs/ovs_datapath.yaml
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -108,7 +108,9 @@ attribute-sets:
         sub-type: u32
       -
         name: ifindex
-        type: u32
+        type: s32
+        checks:
+          min: 0
 
 operations:
   fixed-header: ovs-header
@@ -119,6 +121,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 +138,54 @@ 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. A
+        successful set also sends the updated data path to the
+        family's multicast group, and to the requester when the
+        request sets NLM_F_ECHO.
+      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..af7ee56f5f90 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,27 @@ 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. A successful set
+        also sends the updated vport to the family's multicast group,
+        and to the requester when the request sets NLM_F_ECHO.
+      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

[PATCH net-next v8 3/3] netlink: specs: correct the ovs specs where they do not match the kernel

[Minxi Hou]

The vport netnsid attribute is signed in the kernel: vport_policy declares
it NLA_S32 and ovs_vport_cmd_fill_info() sends it with nla_put_s32(). A
client reading it as unsigned turns the -1 that peernet2id_alloc() returns
when it cannot assign an identifier into 4294967295. Declare it s32, and
say the same in the uapi comment, which also never said the value is -1
when no identifier could be assigned.

Two dumps shared a request list with their get operation, while the dump
handlers read no request attribute. Each dump now has a request list of its
own with no attributes, so neither advertises an attribute the kernel would
ignore.

Four lists named attributes their handler never reads or sends. The vport
delete goes through lookup_vport() and reads no type. The datapath reply
does not carry upcall-pid, which the uapi reserves for the new request, so
the reply list drops it and the ynl selftest stops printing that field. The
vport new request named the nested options, which the kernel accepts and
ignores. The packet miss event shared its list with the userspace action
event, which alone fills userdata, actions, the tunnel key and the length
of a truncated packet, so the miss event now lists only what a miss sends.

The vport get request can also be answered by port number, because
lookup_vport() falls back to it when the request carries no name, so list
port-no there. The datapath get is served by the name, or by the header
ifindex when the name is absent, which its doc now says, and all four ovs
specs document the header struct the same way. The vport new doc gains the
constraint that an ifindex is only accepted for an internal vport.

The uapi note on the vport name now covers requests other than new and
dump, and names the header member the lookup uses. The note on options says
what the kernel does with the attribute instead of leaving it to the vport
type.

The packet spec declared neither the validate flags nor the admin
permission that the kernel registers on the execute operation.

The vport spec also declared family version 2 where the kernel registers 1.

None of this comes from the set operations added earlier in the series; the
specs have been wrong since the families were introduced.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
 Documentation/netlink/specs/ovs_datapath.yaml | 16 +++++++++----
 Documentation/netlink/specs/ovs_packet.yaml   | 20 +++++++++++-----
 Documentation/netlink/specs/ovs_vport.yaml    | 24 ++++++++++++-------
 include/uapi/linux/openvswitch.h              | 14 ++++++-----
 tools/net/ynl/tests/ovs.c                     |  5 ++--
 5 files changed, 51 insertions(+), 28 deletions(-)

diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
index 6232760224dc..94ab15251e2a 100644
--- a/Documentation/netlink/specs/ovs_datapath.yaml
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -12,6 +12,8 @@ definitions:
   -
     name: ovs-header
     type: struct
+    doc: |
+      Header for OVS Generic Netlink messages.
     members:
       -
         name: dp-ifindex
@@ -118,24 +120,28 @@ operations:
   list:
     -
       name: get
-      doc: Get / dump OVS data path configuration and state
+      doc: |
+        Get one OVS data path by name, or by the ifindex in the header when
+        the request carries no name.
       value: 3
       attribute-set: datapath
       dont-validate: [strict, dump]
-      do: &dp-get-op
+      do:
         request:
           attributes:
             - name
-        reply:
+        reply: &dp-reply
           attributes:
             - name
-            - upcall-pid
             - stats
             - megaflow-stats
             - user-features
             - masks-cache-size
             - per-cpu-pids
-      dump: *dp-get-op
+      dump:
+        request:
+          attributes: []
+        reply: *dp-reply
     -
       name: new
       doc: |
diff --git a/Documentation/netlink/specs/ovs_packet.yaml b/Documentation/netlink/specs/ovs_packet.yaml
index e4c5f24b3b01..a9ee84be6e22 100644
--- a/Documentation/netlink/specs/ovs_packet.yaml
+++ b/Documentation/netlink/specs/ovs_packet.yaml
@@ -22,6 +22,8 @@ definitions:
   -
     name: ovs-header
     type: struct
+    doc: |
+      Header for OVS Generic Netlink messages.
     members:
       -
         name: dp-ifindex
@@ -97,14 +99,10 @@ operations:
       value: 1
       attribute-set: packet
       event:
-        attributes: &event-attrs
+        attributes:
           - packet
           - key
-          - userdata
-          - actions
-          - egress-tun-key
           - mru
-          - len
           - hash
     -
       name: action
@@ -112,12 +110,22 @@ operations:
       value: 2
       attribute-set: packet
       event:
-        attributes: *event-attrs
+        attributes: &event-attrs
+          - packet
+          - key
+          - userdata
+          - actions
+          - egress-tun-key
+          - mru
+          - len
+          - hash
     -
       name: execute
       doc: Apply actions to a packet.
       value: 3
       attribute-set: packet
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
index af7ee56f5f90..7626b578c568 100644
--- a/Documentation/netlink/specs/ovs_vport.yaml
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
 ---
 name: ovs_vport
-version: 2
+version: 1
 protocol: genetlink-legacy
 uapi-header: linux/openvswitch.h
 
@@ -12,6 +12,8 @@ definitions:
   -
     name: ovs-header
     type: struct
+    doc: |
+      Header for OVS Generic Netlink messages.
     members:
       -
         name: dp-ifindex
@@ -116,7 +118,7 @@ attribute-sets:
           min: 0
       -
         name: netnsid
-        type: u32
+        type: s32
       -
         name: upcall-stats
         type: nest
@@ -128,7 +130,9 @@ operations:
   list:
     -
       name: new
-      doc: Create a new OVS vport
+      doc: |
+        Create a new OVS vport. An ifindex is accepted only for an
+        internal vport; any other type returns -EOPNOTSUPP.
       attribute-set: vport
       dont-validate: [strict, dump]
       flags: [uns-admin-perm]
@@ -140,7 +144,6 @@ operations:
             - upcall-pid
             - port-no
             - ifindex
-            - options
     -
       name: del
       doc: Delete existing OVS vport from a data path
@@ -151,17 +154,19 @@ operations:
         request:
           attributes:
             - port-no
-            - type
             - name
     -
       name: get
-      doc: Get / dump OVS vport configuration and state
+      doc: |
+        Get one OVS vport by name, or by port number when the request
+        carries no name.
       attribute-set: vport
       dont-validate: [strict, dump]
-      do: &vport-get-op
+      do:
         request:
           attributes:
             - name
+            - port-no
         reply: &dev-all
           attributes:
             - port-no
@@ -172,7 +177,10 @@ operations:
             - ifindex
             - netnsid
             - upcall-stats
-      dump: *vport-get-op
+      dump:
+        request:
+          attributes: []
+        reply: *dev-all
     -
       name: set
       doc: |
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index bd77e6fb4a95..b111eb9a0066 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -293,7 +293,9 @@ enum ovs_vport_type {
  * packets sent or received through the vport.
  * @OVS_VPORT_ATTR_IFINDEX: Provides the ifindex of a vport, or sets the desired
  * ifindex while creating a new vport with type %OVS_VPORT_TYPE_INTERNAL.
- * @OVS_VPORT_ATTR_NETNSID: Provides the netns id of the vport if it's not local.
+ * @OVS_VPORT_ATTR_NETNSID: Provides the netns id of the vport if it's not
+ * local, as a signed 32-bit value.  The value is -1 when the identifier could
+ * not be assigned.
  * @OVS_VPORT_ATTR_UPCALL_STATS: Provides upcall statistics for a vport.
  * Contains nested %OVS_VPORT_UPCALL_ATTR_* attributes.
  *
@@ -303,12 +305,12 @@ enum ovs_vport_type {
  * For %OVS_VPORT_CMD_NEW requests, the %OVS_VPORT_ATTR_TYPE and
  * %OVS_VPORT_ATTR_NAME attributes are required.  %OVS_VPORT_ATTR_PORT_NO is
  * optional; if not specified a free port number is automatically selected.
- * Whether %OVS_VPORT_ATTR_OPTIONS is required or optional depends on the type
- * of vport.  None of currently supported vport types support options.
+ * None of currently supported vport types support options, so
+ * %OVS_VPORT_ATTR_OPTIONS is ignored in a %OVS_VPORT_CMD_NEW request and
+ * rejected with -EOPNOTSUPP in a %OVS_VPORT_CMD_SET request.
  *
- * For other requests, if %OVS_VPORT_ATTR_NAME is specified then it is used to
- * look up the vport to operate on; otherwise dp_idx from the &struct
- * ovs_header plus %OVS_VPORT_ATTR_PORT_NO determine the vport.
+ * For requests other than new and dump, if %OVS_VPORT_ATTR_NAME is specified
+ * then it is used to look up the vport to operate on; otherwise dp_ifindex from
  */
 enum ovs_vport_attr {
 	/* private: */
diff --git a/tools/net/ynl/tests/ovs.c b/tools/net/ynl/tests/ovs.c
index d49f5a8e647e..fea7609a68b9 100644
--- a/tools/net/ynl/tests/ovs.c
+++ b/tools/net/ynl/tests/ovs.c
@@ -16,9 +16,8 @@ static void ovs_print_datapath(struct __test_metadata *_metadata,
 		return;
 
 	EXPECT_TRUE((bool)dp->_hdr.dp_ifindex);
-	ksft_print_msg("%s(%d): pid:%u cache:%u\n",
-		       dp->name, dp->_hdr.dp_ifindex,
-		       dp->upcall_pid, dp->masks_cache_size);
+	ksft_print_msg("%s(%d): cache:%u\n",
+		       dp->name, dp->_hdr.dp_ifindex, dp->masks_cache_size);
 }
 
 FIXTURE(ovs)
-- 
2.56.0