Re: [PATCH net v4] openvswitch: cap upcall PID array size and pre-size vport replies

[Ilya Maximets <i.maximets@ovn.org>]

On 4/15/26 2:51 PM, Weiming Shi wrote:
> The vport netlink reply helpers allocate a fixed-size skb with
> nlmsg_new(NLMSG_DEFAULT_SIZE, ...) but serialize the full upcall PID
> array via ovs_vport_get_upcall_portids().  Since
> ovs_vport_set_upcall_portids() accepts any non-zero multiple of
> sizeof(u32) with no upper bound, a CAP_NET_ADMIN user can install a PID
> array large enough to overflow the reply buffer, causing nla_put() to
> fail with -EMSGSIZE and hitting BUG_ON(err < 0).  On systems with
> unprivileged user namespaces enabled (e.g., Ubuntu default), this is
> reachable via unshare -Urn since OVS vport mutation operations use
> GENL_UNS_ADMIN_PERM.
> 
>  kernel BUG at net/openvswitch/datapath.c:2414!
>  Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>  CPU: 1 UID: 0 PID: 65 Comm: poc Not tainted 7.0.0-rc7-00195-geb216e422044 #1
>  RIP: 0010:ovs_vport_cmd_set+0x34c/0x400
>  Call Trace:
>   <TASK>
>   genl_family_rcv_msg_doit (net/netlink/genetlink.c:1116)
>   genl_rcv_msg (net/netlink/genetlink.c:1194)
>   netlink_rcv_skb (net/netlink/af_netlink.c:2550)
>   genl_rcv (net/netlink/genetlink.c:1219)
>   netlink_unicast (net/netlink/af_netlink.c:1344)
>   netlink_sendmsg (net/netlink/af_netlink.c:1894)
>   __sys_sendto (net/socket.c:2206)
>   __x64_sys_sendto (net/socket.c:2209)
>   do_syscall_64 (arch/x86/entry/syscall_64.c:63)
>   entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
>   </TASK>
>  Kernel panic - not syncing: Fatal exception
> 
> Reject attempts to set more PIDs than nr_cpu_ids in
> ovs_vport_set_upcall_portids(), and pre-compute the worst-case reply
> size in ovs_vport_cmd_msg_size() based on that bound, similar to the
> existing ovs_dp_cmd_msg_size().  nr_cpu_ids matches the cap already
> used by the per-CPU dispatch configuration on the datapath side
> (ovs_dp_cmd_fill_info() serialises at most nr_cpu_ids PIDs), so the
> two sides stay consistent.
> 
> Fixes: 5cd667b0a456 ("openvswitch: Allow each vport to have an array of 'port_id's.")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
> v4 (per Ilya):
> - Use nr_cpu_ids instead of num_possible_cpus() for consistency with
>   the per-CPU dispatch on the datapath side.
> - Annotate ovs_vport_cmd_msg_size() per-attribute; split nested sums.
> v3: Cap at num_possible_cpus(); add ovs_vport_cmd_msg_size(); keep
>     BUG_ON(); fix Fixes tag.
> v2: Dynamically size reply skb; drop WARN_ON_ONCE, return plain errors.

Please, don't re-name the patch for every version if there are no changes
that actually invalidate the name.  It was definitely not necessary in the
past few versions of this patch.  Could've even kept the original name
from v1, it was fine.  But please, keep the current v4 name in v5.

These renames are messing up version tracking.  Also, please, add lore links
into the changelog for previous versions, especially if you're renaming the
patch, so reviewers can find the older versions.

In case you're using AI to help with these patches (which would explain the
constant renaming), you should disclose that by adding an Assisted-by tag:
  https://docs.kernel.org/process/coding-assistants.html#attribution

> ---
>  net/openvswitch/datapath.c | 33 +++++++++++++++++++++++++++++++--
>  net/openvswitch/vport.c    |  3 +++
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index e209099218b4..35e67e51b0d2 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -2184,9 +2184,38 @@ static int ovs_vport_cmd_fill_info(struct vport *vport, struct sk_buff *skb,
>  	return err;
>  }
>  
> +static size_t ovs_vport_cmd_msg_size(void)
> +{
> +	size_t msgsize = NLMSG_ALIGN(sizeof(struct ovs_header));
> +
> +	msgsize += nla_total_size(sizeof(u32)); /* OVS_VPORT_ATTR_PORT_NO */
> +	msgsize += nla_total_size(sizeof(u32)); /* OVS_VPORT_ATTR_TYPE */
> +	msgsize += nla_total_size(IFNAMSIZ);    /* OVS_VPORT_ATTR_NAME */
> +	msgsize += nla_total_size(sizeof(u32)); /* OVS_VPORT_ATTR_IFINDEX */
> +	msgsize += nla_total_size(sizeof(s32)); /* OVS_VPORT_ATTR_NETNSID */

Add an empty line here, it's hard to read when comments are sandwiched
between the code.  Same for all the blocks below (empty line before the
comment line).

> +	/* OVS_VPORT_ATTR_STATS */
> +	msgsize += nla_total_size_64bit(sizeof(struct ovs_vport_stats));
> +	/* OVS_VPORT_ATTR_UPCALL_STATS(OVS_VPORT_UPCALL_ATTR_SUCCESS +
> +	 *                             OVS_VPORT_UPCALL_ATTR_FAIL)
> +	 */
> +	msgsize += nla_total_size(nla_total_size_64bit(sizeof(u64)) +
> +				  nla_total_size_64bit(sizeof(u64)));
> +	/* OVS_VPORT_ATTR_UPCALL_PID (capped at nr_cpu_ids by
> +	 * ovs_vport_set_upcall_portids())

The explanation inside the parentheses is not needed, IMO.

The rest seems fine to me.

Best regards, Ilya Maximets.