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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netlink: specs: add ovs_flow del and set operations

This patch adds the del and set operations to the ovs_flow YNL spec. It
marks new, del and set as needing admin permission and lists the
attributes each command reads. It also changes probe from binary to flag
to match the kernel policy, and rewrites the doc for the key attribute.

> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 951837b72e1d..e5e110b6ccc8 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -384,7 +384,9 @@ attribute-sets:
>          nested-attributes: key-attrs
>          doc: |
>            Nested attributes specifying the flow key. Always present in
> -          notifications. Required for all requests (except dumps).
> +          notifications. A request may carry a ufid instead, except for a
> +          new flow, which needs a key. A set that replaces the actions
> +          needs a key too.

[Severity: Low]
Should include/uapi/linux/openvswitch.h get the same update? The
kernel-doc for enum ovs_flow_attr still says:

 * @OVS_FLOW_ATTR_KEY: Nested %OVS_KEY_ATTR_* attributes specifying the flow
 * key.  Always present in notifications.  Required for all requests (except
 * dumps).

The spec text was first copied from that header. After this patch the two
in-tree descriptions of this attribute disagree, and the header still
describes a rule the kernel does not enforce.

ovs_flow_cmd_set() only rejects the request when both the key and the ufid
are missing:

	if (!a[OVS_FLOW_ATTR_KEY] && !ufid_present) {
		OVS_NLERR(log,
			  "Flow set message rejected, Key attribute missing.");
		return -EINVAL;
	}

ovs_flow_cmd_get() does the same:

	} else if (!ufid_present) {
		OVS_NLERR(log,
			  "Flow get message rejected, Key attribute missing.");
		err = -EINVAL;
	}

The new wording also drops the "except dumps" exception. "A request may
carry a ufid instead" suggests that every request other than new needs
either a key or a ufid. But ovs_flow_cmd_dump() reads neither, and a del
that carries neither is a valid flush of the data path.

Could the doc mention those two cases as well?

[Severity: Low]
This is a pre-existing issue, not one this patch introduced, but is
"Always present in notifications" accurate? This patch keeps the sentence
in the paragraph it rewrites, and the same text is in
include/uapi/linux/openvswitch.h.

ovs_flow_cmd_fill_info() calls ovs_nla_put_identifier(). For a
ufid-identified flow, that emits OVS_FLOW_ATTR_UFID instead of the key:

	if (ovs_identifier_is_ufid(&flow->id))
		return nla_put(skb, OVS_FLOW_ATTR_UFID, flow->id.ufid_len,
			       flow->id.ufid);

The key is then added only if should_fill_key() returns true:

	return ovs_identifier_is_ufid(sfid) &&
	       !(ufid_flags & OVS_UFID_F_OMIT_KEY);

So when the flow is ufid-identified and the request sets
OVS_UFID_F_OMIT_KEY in ufid-flags, the reply or notification has no key.

[ ... ]

> @@ -438,7 +440,7 @@ attribute-sets:
>            are exact match bits.
>        -
>          name: probe
> -        type: binary
> +        type: flag

[Severity: Low]
This corrects an existing mismatch with the kernel policy in flow_policy:

	[OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },

The binary type came in with the commit that added ovs_flow.yaml. Should
this change carry a tag like:

Fixes: 93b230b549bc ("netlink: specs: add ynl spec for ovs_flow")

Could it also go in a separate patch, so it can be picked up or backported
apart from the new del and set operations?

>          doc: |
>            Flow operation is a feature probe, error logging should be suppressed.

[ ... ]
-- 
pw-bot: cr