Re: [PATCH net-next 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 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 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 new `set` op in Documentation/netlink/specs/ovs_flow.yaml
  lists `probe` as a request attribute.
- [Low] The new `del` op in Documentation/netlink/specs/ovs_flow.yaml
  lists only key, ufid and ufid-flags.
- [Low] The new `del` and `set` ops in ovs_flow.yaml declare only
  `do.request`: no `reply`, and no op with `notify: del`/`notify: set`.
- [Low] The `set` op doc says: "The kernel rejects the request with
  -EINVAL when it carries neither a key nor a ufid." Together with the…

--

netlink: specs: add ovs_flow del and set operations

This patch adds del and set operations to the ovs_flow YNL spec. It also
marks new, del and set with uns-admin-perm. It lists the request
attributes each command reads.

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

[ ... ]

> @@ -995,6 +996,36 @@ operations:
>              - ufid
>              - mask
>              - actions
> +    -
> +      name: del
> +      doc: Delete one flow, or every flow in the data path
> +      value: 2
> +      attribute-set: flow-attrs
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - key
> +            - ufid
> +            - ufid-flags

[Severity: Low]
Should probe be listed here too? ovs_flow_cmd_del() reads it:

net/openvswitch/datapath.c:ovs_flow_cmd_del() {
	...
	bool log = !a[OVS_FLOW_ATTR_PROBE];
	bool ufid_present;

	ufid_present = ovs_nla_get_ufid(&ufid, a[OVS_FLOW_ATTR_UFID], log);
	...
}

log is then passed to ovs_nla_get_ufid() and ovs_nla_get_match().

The commit message says "Name the attributes each command reads", and the
set op in this patch does list probe.

C YNL only generates setters for request attributes that are listed. As a
result, a C client cannot mark a delete as a feature probe to suppress
the kernel's error logging.

[Severity: Low]
Neither del nor set describes a reply or a notification. Does the kernel
send messages for these commands that the spec doesn't cover?

ovs_flow_cmd_del() uses ovs_flow_cmd_fill_info() to build an
OVS_FLOW_CMD_DEL message. It carries the deleted flow's key, ufid, mask,
stats, actions, used and tcp-flags.

ovs_flow_cmd_set() builds an OVS_FLOW_CMD_SET message with
ovs_flow_cmd_build_info(). Both handlers send through ovs_notify(), gated
by ovs_must_notify():

net/openvswitch/datapath.c:ovs_must_notify() {
	return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
	       genl_has_listeners(family, genl_info_net(info), group);
}

A client may set NLM_F_ECHO to get the final stats of a deleted flow, or
subscribe to the ovs_flow mcast group. In both cases the spec gives it
nothing to decode these messages with.

The existing new op and the ovs_datapath and ovs_vport specs have the
same gap, so this follows existing practice in the OVS specs. Still, the
new ops describe less than the kernel sends. Could a reply or notify
description be added for del and set?

> +    -
> +      name: set
> +      doc: |
> +        Modify an existing flow. The kernel rejects the request with
> +        -EINVAL when it carries neither a key nor a ufid.

[Severity: Low]
Is this doc complete? The request list below includes both ufid and
actions, so the doc reads as though a ufid alone is enough for any set.

After the key/ufid check, ovs_flow_cmd_set() calls
ovs_nla_init_match_and_action(). That function rejects actions without a
key, even when a ufid is present:

net/openvswitch/datapath.c:ovs_nla_init_match_and_action() {
	...
	if (a[OVS_FLOW_ATTR_ACTIONS]) {
		if (!a[OVS_FLOW_ATTR_KEY]) {
			OVS_NLERR(log,
				  "Flow key attribute not present in set flow.");
			error = -EINVAL;
			goto error;
		}
	...
}

A set that carries only a ufid works for stats or clear updates. One that
replaces actions always gets -EINVAL. Could the doc say that a key is
required when actions are supplied?

> +      value: 4
> +      attribute-set: flow-attrs
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - key
> +            - ufid
> +            - mask
> +            - actions
> +            - ufid-flags
> +            - clear
> +            - probe

[Severity: Medium]
Should probe be declared as type: flag in flow-attrs? The attribute set
currently has:

        name: probe
        type: binary

The kernel policy treats it as a flag:

net/openvswitch/datapath.c:flow_policy[] = {
	...
	[OVS_FLOW_ATTR_CLEAR] = { .type = NLA_FLAG },
	[OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },
	...
}

validate_nla() in lib/nlattr.c rejects an NLA_FLAG attribute that carries
any payload:

	case NLA_FLAG:
		if (attrlen > 0)
			goto out_err;

Before this patch no op listed probe in a request, so the mismatch was not
visible. Now that set lists it, C YNL codegen will emit a pointer plus
length binary setter for probe. Any non-empty payload makes the whole
OVS_FLOW_CMD_SET fail policy validation with -EINVAL.

In Python YNL, passing "probe": true goes to the binary branch of
_add_attr() in tools/net/ynl/pyynl/lib/ynl.py. That branch raises
YnlException('Unknown type for binary attribute ...'), while the flag
branch would accept the value.

Could probe be changed to type: flag, the same as clear?

>  
>  mcast-groups:
>    list:

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