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

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

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 | 35 +++++++-
 Documentation/netlink/specs/ovs_flow.yaml     | 86 ++++++++++++++++---
 Documentation/netlink/specs/ovs_vport.yaml    | 30 ++++++-
 include/uapi/linux/openvswitch.h              | 23 +++--
 4 files changed, 150 insertions(+), 24 deletions(-)


base-commit: fa5196392691051963726787944db872420dfa75
-- 
2.56.0

[PATCH net-next v6 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 no key
and no valid ufid, 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. A YNL client leaves
out a false flag, where it sent an empty binary attribute before.

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, and a del that carries no key and no valid ufid flushes the data
path.

The ufid doc promised that the flow is indexed by that value, which does
not hold for a value the kernel rejects. Anything outside 1-16 octets is
read as if the attribute were absent, so a del with such a ufid and no
key flushes the data path, and a new request indexes the flow by its key
instead. Say so, and record the minimum length the policy enforces in the
attribute.

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 | 86 +++++++++++++++++++----
 include/uapi/linux/openvswitch.h          | 23 ++++--
 2 files changed, 88 insertions(+), 21 deletions(-)

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 951837b72e1d..bfdbf55bc219 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -383,19 +383,27 @@ 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 no key and no valid ufid flushes the data path.
       -
         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
@@ -438,18 +446,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 request that carries a
+          value outside that range is treated as if it carried no ufid, so a
+          new flow is indexed by its key. Optional for all requests. Present
+          in notifications if the flow was created with this attribute.
+          Ignored in a dump.
         display-hint: uuid
       -
         name: ufid-flags
@@ -969,25 +981,33 @@ 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
             - stats
             - 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 +1015,44 @@ operations:
             - ufid
             - mask
             - actions
+            - ufid-flags
+            - probe
+    -
+      name: del
+      doc: |
+        Delete one flow, or every flow in the data path. A request that
+        carries no key and no valid ufid flushes the data path.
+      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..4c5625c8c1cf 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -624,11 +624,18 @@ 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 no key and
+ * no valid ufid flushes the data path.
  * @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.
@@ -655,9 +662,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
- * requests. Present in notifications if the flow was created with this
- * attribute.
+ * than the value of the %OVS_FLOW_ATTR_KEY attribute. A request that carries
+ * a value outside that range is treated as if it carried no ufid, so a new
+ * flow is indexed by its key. Optional for all requests. Present in
+ * notifications if the flow was created with this attribute. Ignored in a
+ * dump.
  * @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 v6 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 new operation reads per-cpu-pids the same way, so its
doc says so too.

The vport SET command changes the upcall pid. It finds the vport by name
or by port number, and rejects an ifindex with -EOPNOTSUPP before it
looks at the type or the options. A type that differs from the vport's
type is rejected with -EINVAL, and options with -EOPNOTSUPP.

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 with a minimum of 0 rather than u32. A generated client would
otherwise offer a setter for values the kernel rejects with -ERANGE.

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 | 35 +++++++++++++++++--
 Documentation/netlink/specs/ovs_vport.yaml    | 30 +++++++++++++++-
 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..4a64985633c4 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,52 @@ 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
+        user-features in the same request sets
+        OVS_DP_F_DISPATCH_UPCALL_PER_CPU.
       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..16b21c42cac8 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,6 +130,8 @@ operations:
       name: new
       doc: Create a new OVS vport
       attribute-set: vport
+      dont-validate: [strict, dump]
+      flags: [uns-admin-perm]
       do:
         request:
           attributes:
@@ -140,6 +144,8 @@ operations:
       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 +156,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 +172,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. The kernel rejects an ifindex with -EOPNOTSUPP, before
+        it looks at the type or the options. It rejects a type that differs
+        from the vport's type with -EINVAL, and rejects options with
+        -EOPNOTSUPP. The only 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
+            - options
+            - upcall-pid
+            - port-no
+            - ifindex
 
 mcast-groups:
   list:
-- 
2.56.0

Re: [PATCH net-next v6 1/2] netlink: specs: add ovs_flow del and 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 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new ufid text in Documentation/netlink/specs/ovs_flow.yaml
  (flow-attrs/ufid) and in include/uapi/linux/openvswitch.h…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the get
  reply list, now anchored as &flow-get-reply and reused for the dump…

--

netlink: specs: add ovs_flow del and set operations

This adds the del and set operations to the ovs_flow YNL spec. It also
gives the dump its own request list, changes probe to a flag, and marks
the ops dont-validate and admin-only. The key, actions and ufid docs are
rewritten 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..bfdbf55bc219b 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml

[ ... ]

> @@ -438,18 +446,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 request that carries a
> +          value outside that range is treated as if it carried no ufid, so a
> +          new flow is indexed by its key. Optional for all requests. Present
> +          in notifications if the flow was created with this attribute.
> +          Ignored in a dump.

[Severity: Medium]
Is this accurate for a zero-length ufid? flow_policy in
net/openvswitch/datapath.c has:

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

and validate_nla() in lib/nlattr.c checks the minimum length even when
strict validation is skipped:

    case NLA_UNSPEC:
        if (validate & NL_VALIDATE_UNSPEC) {
            ...
        }
        if (attrlen < pt->len)
            goto out_err;

So for new, del, set and get, the genl core returns -ERANGE for a 0-byte
ufid before any handler runs. A del with a 0-byte ufid and no key then
gets -ERANGE and does not flush the data path.

For dumps the core skips validation. However, ovs_flow_cmd_dump() parses
with the same policy and returns the error:

    err = genlmsg_parse_deprecated(cb->nlh, &dp_flow_genl_family, a,
                                   OVS_FLOW_ATTR_MAX, flow_policy, NULL);
    if (err)
        return err;

So a 0-byte ufid makes the whole dump fail. It is not ignored.

The treated-as-absent path in get_ufid_len() in
net/openvswitch/flow_netlink.c is:

    if (len < 1 || len > MAX_UFID_LENGTH) {

After policy validation, only the len > 16 case can reach it.

The min-len: 1 check added just above also says 0 octets is rejected,
which contradicts the doc text under it. The commit message has the same
contradiction:

  "Anything outside 1-16 octets is read as if the attribute were absent,
  so a del with such a ufid and no key flushes the data path"

next to "record the minimum length the policy enforces in the attribute".

The same claim also appears in these places:

  - the del op doc ("A request that carries no key and no valid ufid
    flushes the data path")
  - the new key doc
  - @OVS_FLOW_ATTR_UFID and @OVS_FLOW_ATTR_KEY in
    include/uapi/linux/openvswitch.h

The later patch in the series ("netlink: specs: add ovs datapath and
vport set operations") does not touch ovs_flow.yaml or openvswitch.h, so
this text is still there at the end of the series.

Could the docs say that a value longer than 16 octets is treated as
absent, and that a zero-length value is rejected with -ERANGE?

>          display-hint: uuid

[ ... ]

> @@ -969,25 +981,33 @@ 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
>              - stats
>              - actions
> -      dump: *flow-get-op
> +      dump:
> +        request:
> +          attributes:
> +            - ufid-flags
> +        reply: *flow-get-reply

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. This
reply list is now anchored and reused for the dump, so should it also
list used and tcp-flags?

ovs_flow_cmd_fill_info() always calls ovs_flow_cmd_fill_stats(). That
function puts both attributes in get replies and dump entries:

    if (used &&
        nla_put_u64_64bit(skb, OVS_FLOW_ATTR_USED, ovs_flow_used_time(used),
                          OVS_FLOW_ATTR_PAD))
        return -EMSGSIZE;
    ...
    if ((u8)ntohs(tcp_flags) &&
         nla_put_u8(skb, OVS_FLOW_ATTR_TCP_FLAGS, (u8)ntohs(tcp_flags)))
        return -EMSGSIZE;

Because they are not in the reply list, YNL-generated clients do not
parse the last-used time or the TCP flags. The dump already shared this
reply list through *flow-get-op before this patch.

[ ... ]

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

Re: [PATCH net-next v6 2/2] netlink: specs: add ovs datapath and vport 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 vport `set` operation in
  Documentation/netlink/specs/ovs_vport.yaml lists `options` and…
- [Low] The commit message says ifindex is declared as s32 with `checks:
  min: 0` because 'A generated client would otherwise offer a setter for…

--

netlink: specs: add ovs datapath and vport set operations

This adds the SET operation to the ovs_datapath and ovs_vport netlink
specs. It also adds attributes to the datapath NEW request, marks
new/del/set as uns-admin-perm and marks the ops as dont-validate strict
and dump. The ifindex attribute moves from u32 to s32 with a min: 0 check
in both families.

> diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
> index fa975f8821b6c..16b21c42cac8c 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

[Severity: Low]
The commit message says:

  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 with a minimum of 0 rather than u32. A generated client would
  otherwise offer a setter for values the kernel rejects with -ERANGE.

Does the min: 0 check change anything a generated client offers? In
tools/net/ynl/pyynl/ynl_gen_c.py, checks are only used to build kernel
policies:

        # Classic families have some funny enums, don't bother
        # computing checks, since we only need them for kernel policies

The OVS kernel policies are not generated from these specs. They are
hand-written in net/openvswitch/datapath.c and already contain:

	[OVS_DP_ATTR_IFINDEX] = NLA_POLICY_MIN(NLA_S32, 0),
	...
	[OVS_VPORT_ATTR_IFINDEX] = NLA_POLICY_MIN(NLA_S32, 0),

So min: 0 has no effect anywhere. The generated s32 setter still accepts
every negative value. Those are the same 32-bit patterns (0x80000000 to
0xffffffff) that the old u32 setter could send, so a generated client
still offers the same rejected values as before.

There is also a side effect the message doesn't mention. The vport ifindex
attribute is part of the GET reply, and ovs_vport_cmd_fill_info() still
sends it as unsigned:

	    nla_put_u32(skb, OVS_VPORT_ATTR_IFINDEX, vport->dev->ifindex))

With this change, the ifindex member of the generated ovs_vport_get_rsp
struct changes from __u32 to __s32. Nothing changes on the wire, since
both types are 4 bytes and dev->ifindex is always positive.

Could the commit message give the actual benefit, which is matching
NLA_S32 and nla_get_s32_default() in the request path? Could it also
mention the type change in the reply? The same rationale also appears for
the ovs_datapath.yaml change.

[ ... ]

> @@ -165,6 +172,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. The kernel rejects an ifindex with -EOPNOTSUPP, before
> +        it looks at the type or the options. It rejects a type that differs
> +        from the vport's type with -EINVAL, and rejects options with
> +        -EOPNOTSUPP. The only 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
> +            - options
> +            - upcall-pid
> +            - port-no
> +            - ifindex

[Severity: Low]
Should options and ifindex be in the set request list? The kernel always
rejects both of them:

net/openvswitch/datapath.c:lookup_vport() {
	if (a[OVS_VPORT_ATTR_IFINDEX])
		return ERR_PTR(-EOPNOTSUPP);
    ...
}

net/openvswitch/datapath.c:ovs_vport_cmd_set() {
    ...
	if (a[OVS_VPORT_ATTR_OPTIONS]) {
		/* There are no vport types that support legacy options. */
		err = -EOPNOTSUPP;
		goto exit_unlock_free;
    ...
}

The vport family is code-generated (tools/net/ynl/Makefile.deps has
CFLAGS_ovs_vport). That means ynl_gen_c.py will emit set request setters
for ifindex and for the options nest. Any request that uses them can only
fail with -EOPNOTSUPP.

This seems to go against the reason the commit message gives for retyping
ifindex: "A generated client would otherwise offer a setter for values the
kernel rejects". The get and del ops go through the same lookup_vport(),
and this patch leaves ifindex out of them.

Would a request list of name, port-no, type (used as a match check) and
upcall-pid better describe what the kernel accepts?

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

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

[Minxi Hou]

On 2026-11-08 12:39, Sashiko wrote:
> [Medium] The ufid doc promises that the flow is indexed by that value,
> which does not hold for a value the kernel rejects.

v7 rewrites that paragraph. A value longer than 16 octets is read as if
the attribute were absent, so a del that carries such a ufid and no key
flushes the data path, and a new request indexes the flow by its key
instead. A zero-length value never reaches the handler, because the core
enforces the minimum length of one even when strict validation is
skipped, and a dump that parses the same policy fails the same way. The
spec records that minimum now.

Measured on the current tree, so it is not from the code alone: a GET
with an empty ufid returns -ERANGE and the same GET with four octets
returns -ENOENT, which means the attribute does reach the kernel.

> [Medium] The get reply also carries the last-used time and the TCP
> flags, which the reply list omitted.

Added. ovs_flow_cmd_fill_stats() adds both for a get and for a dump, and
the reply list names them in the order the helper writes them.

pw-bot: cr

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

[Minxi Hou]

On 2026-11-08 12:39, Sashiko wrote:
> [Low] The min length has no effect for this family.
> [Low] The vport set list names attributes the kernel rejects.

On the first one, the mechanism is in the generator: TypeScalar.__init__
calls _init_checks() only when the family is not classic
(tools/net/ynl/pyynl/ynl_gen_c.py:375), so for these families a check
documents the kernel policy and nothing else. The kernel enforces it with
NLA_POLICY_MIN(NLA_S32, 0). v7 keeps the declaration and changes the type
to s32, which does reach the generated request code: it now calls
ynl_attr_put_s32() for the attribute.

One disclosure on the same point. v7 also adds a max check on
masks-cache-size, for the same reason, and it does not reach the
generated code either. If a check that generates nothing is worse than no
check, say so and I will drop both rather than leave them in the spec.

The vport set list no longer carries the ifindex or the options, and the
doc says that a request with either of them is rejected.

pw-bot: cr