Re: [PATCH net-next v3 2/2] netlink: specs: add ovs datapath and vport 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 datapath and vport set operations

This adds the SET operation (cmd value 4) to the ovs_datapath and
ovs_vport YNL specs, so YNL clients can modify an existing datapath or
vport. It also adds the uns-admin-perm flag to the new, del and set
operations of both families, matching GENL_UNS_ADMIN_PERM in the kernel.

> diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
> index f7b3671991e6..9c33d6a2f05d 100644
> --- a/Documentation/netlink/specs/ovs_datapath.yaml
> +++ b/Documentation/netlink/specs/ovs_datapath.yaml

[ ... ]

> @@ -149,10 +150,27 @@ operations:

[ ... ]

> +    -
> +      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.
> +      value: 4
> +      attribute-set: datapath
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - name
> +            - user-features
> +            - masks-cache-size
> +            - per-cpu-pids

[Severity: Low]
Should this doc also say that per-cpu-pids only takes effect when the
same request sets OVS_DP_F_DISPATCH_UPCALL_PER_CPU in user-features?

From ovs_dp_change() in net/openvswitch/datapath.c:

	u32 user_features = 0, old_features = dp->user_features;
	...
	dp->user_features = user_features;

	if (dp->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU &&
	    a[OVS_DP_ATTR_PER_CPU_PIDS]) {
		/* Upcall Netlink Port IDs have been updated */
		err = ovs_dp_set_upcall_portids(dp,
						a[OVS_DP_ATTR_PER_CPU_PIDS]);

A SET request with only name and per-cpu-pids, or with user-features that
lack that flag, still succeeds. The pids are dropped without an error, and
per-CPU upcall dispatch is turned off too.

The kernel behaves this way without this patch. The new op doc describes
the related user-features quirk but not this one.

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Neither
of the new set ops describes the notification the kernel sends.

ovs_dp_cmd_set() and ovs_vport_cmd_set() both build a full object message
with cmd OVS_DP_CMD_SET / OVS_VPORT_CMD_SET and multicast it:

net/openvswitch/datapath.c:ovs_vport_cmd_set() {
    ...
	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
				      info->snd_portid, info->snd_seq, 0,
				      OVS_VPORT_CMD_SET, GFP_KERNEL);
	BUG_ON(err < 0);

	ovs_unlock();
	ovs_notify(&dp_vport_genl_family, reply, info);
    ...
}

The set ops in the specs have only a do request, with no reply and no
notify. C codegen therefore generates no parser for these cmd 4 messages.
The existing new and del ops in both specs have the same gap, although
their handlers also send notifications.

The patch does help Python YNL here. handle_ntf() looks up the op with
rsp_by_value[cmd], so cmd 4 notifications can now be decoded with the op's
attribute set.

Could these notifications be described in the specs, either here or in a
follow-up?

> diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
> index fa975f8821b6..63a0a41a1199 100644
> --- a/Documentation/netlink/specs/ovs_vport.yaml
> +++ b/Documentation/netlink/specs/ovs_vport.yaml

[ ... ]

> @@ -165,6 +167,19 @@ operations:
>              - netnsid
>              - upcall-stats
>        dump: *vport-get-op
> +    -
> +      name: set
> +      doc: Modify an existing OVS vport
> +      value: 4
> +      attribute-set: vport
> +      flags: [uns-admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - name

[Severity: Low]
Should port-no be listed here too?

ovs_vport_cmd_set() finds its target with lookup_vport(). That function
accepts either OVS_VPORT_ATTR_NAME or OVS_VPORT_ATTR_PORT_NO, using the
dp_ifindex from the fixed header:

net/openvswitch/datapath.c:lookup_vport() {
    ...
	} else if (a[OVS_VPORT_ATTR_PORT_NO]) {
		u32 port_no = nla_get_u32(a[OVS_VPORT_ATTR_PORT_NO]);
		...
		dp = get_dp(net, ovs_header->dp_ifindex);
		...
		vport = ovs_vport_ovsl_rcu(dp, port_no);
    ...
}

The del op in this spec already lists port-no. YNL C codegen only
generates setters for the attributes an op lists. So the generated
ovs_vport_set_req has no port_no setter, and a C client can only pick the
vport by name. Python YNL is not affected because it encodes from the
whole attribute set.

> +            - type
> +            - options

[Severity: Low]
Is options meant to be listed for set? ovs_vport_cmd_set() rejects every
request that carries it:

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

vport_policy also marks OVS_VPORT_ATTR_OPTIONS as "/* Unused. */". Any
request built with the generated options setters for this op will fail
with EOPNOTSUPP.

type is similar. The kernel only checks it for equality and cannot change
it:

	if (a[OVS_VPORT_ATTR_TYPE] &&
	    nla_get_u32(a[OVS_VPORT_ATTR_TYPE]) != vport->ops->type) {
		err = -EINVAL;

The commit message says "The kernel rejects a type change and rejects
options". The spec, with its doc "Modify an existing OVS vport", says
neither.

Could options be dropped from this request, or could the doc mention
these limits?