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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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 patch adds `checks: max: 4096` to `masks-cache-size` in
  Documentation/netlink/specs/ovs_datapath.yaml.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  Documentation/netlink/specs/ovs_vport.yaml still declares the vport…
- [Low] This problem wasn't introduced by this patch, but the vport `get`
  do request in Documentation/netlink/specs/ovs_vport.yaml lists only…
- [Low] The new vport and datapath `set` ops in the specs declare no
  reply and no notification.

--

netlink: specs: add ovs datapath and vport set operations

This commit adds SET operations to the ovs_datapath and ovs_vport netlink
specs and adds more attributes to the NEW request lists. It also marks
new, del and set as uns-admin-perm and changes ifindex to s32 with a
minimum of 0. Finally it puts a maximum of 4096 on masks-cache-size and
marks all ops as dont-validate strict and dump.

> diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
> index f7b3671991e6c..b24dc1e03f46a 100644
> --- a/Documentation/netlink/specs/ovs_datapath.yaml
> +++ b/Documentation/netlink/specs/ovs_datapath.yaml
> @@ -102,13 +102,17 @@ attribute-sets:
>        -
>          name: masks-cache-size
>          type: u32
> +        checks:
> +          max: 4096

[Severity: Medium]
Is 4096 the right bound on every configuration? The commit message says:

  The masks cache size has an upper bound of 4096, which the attribute
  records.

However, datapath_policy in net/openvswitch/datapath.c computes the limit
from the percpu unit size:

	[OVS_DP_ATTR_MASKS_CACHE_SIZE] =  NLA_POLICY_RANGE(NLA_U32, 0,
		PCPU_MIN_UNIT_SIZE / sizeof(struct mask_cache_entry)),

and include/linux/percpu.h has:

#define PCPU_MIN_UNIT_SIZE		PFN_ALIGN(32 << 10)

struct mask_cache_entry is 8 bytes. That gives a limit of 4096 on 4K and
16K page kernels, 8192 with 64K pages (ppc64, arm64 with
CONFIG_ARM64_64K_PAGES) and 32768 with 256K pages. On those
configurations the spec would mark values that the kernel accepts as out
of range.

The spec also leaves out a second rule.
ovs_flow_tbl_masks_cache_resize() in net/openvswitch/flow_table.c rejects
any non-zero size that is not a power of two:

	if ((!is_power_of_2(size) && size != 0) ||
	    (size * sizeof(struct mask_cache_entry)) > PCPU_MIN_UNIT_SIZE)
		return -EINVAL;

Could the max check be dropped? Another option is to document the bound
as PCPU_MIN_UNIT_SIZE / 8, along with the zero-or-power-of-two rule.

[ ... ]

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

[Severity: Low]
This isn't a bug introduced by this patch. Since ifindex is being matched
to the kernel policy here, should netnsid be fixed the same way? The spec
still declares netnsid as u32, but vport_policy in
net/openvswitch/datapath.c has:

	[OVS_VPORT_ATTR_NETNSID] = { .type = NLA_S32 },

and ovs_vport_cmd_fill_info() sends it with nla_put_s32():

		int id = peernet2id_alloc(net, net_vport, GFP_ATOMIC);

		if (nla_put_s32(skb, OVS_VPORT_ATTR_NETNSID, id))

peernet2id_alloc() can return NETNSA_NSID_NOT_ASSIGNED (-1). A YNL client
that follows the spec would read that as 4294967295.

[ ... ]

> @@ -150,6 +157,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:

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The get
do request still lists only name. ovs_vport_cmd_get() uses the same
lookup_vport() helper as set and del, and that helper falls back to the
port number when the request has no name:

	} else if (a[OVS_VPORT_ATTR_PORT_NO]) {
		u32 port_no = nla_get_u32(a[OVS_VPORT_ATTR_PORT_NO]);

del and the new set op both list port-no. Should get list it too, so that
clients generated from the spec can look up a vport by port number?

> @@ -165,6 +173,25 @@ 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 when the request carries no name. An ifindex or options
> +        are rejected with -EOPNOTSUPP, and a type that differs from the
> +        vport's type with -EINVAL. The type only has to match; the one
> +        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
> +            - upcall-pid
> +            - port-no

[Severity: Low]
This is a pre-existing gap, and the existing new and del ops have it too.
The new set op here declares no reply or notification, and neither does
the datapath set op in ovs_datapath.yaml.

ovs_vport_cmd_set() builds a full vport message and sends it with
ovs_notify(), which calls genl_notify():

	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);

ovs_dp_cmd_set() does the same with OVS_DP_CMD_SET. These messages go to
the family's multicast group. They also go to the requester when
NLM_F_ECHO is set.

Should the set ops describe these messages, for example with a notify
entry that reuses the get reply, so generated clients can decode them?

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