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

[netdev-bot+sashiko@kernel.org]

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