Re: [PATCH v12 net-next 4/9] devlink: Implement devlink param multi attribute nested data values

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-05-08 at 09:19:07, Ratheesh Kannoth (rkannoth@marvell.com) wrote:
> From: Saeed Mahameed <saeedm@nvidia.com>
>
>Separately, the commit message justifies this new UAPI type only with
>"without breaking any policies".  DEVLINK_VAR_ATTR_TYPE_BINARY already
>exists and can carry an arbitrary-length payload in a single
>attribute.  Could the commit message say why BINARY (with a small
>in-payload framing) was not sufficient, given that a new UAPI enum
>value is permanent?
This is for array of UINT32 or UINT64 types. And commit message clearly mention the same.

>Also, the 32-element cap and the fact that elements are encoded as
>repeated top-level DEVLINK_ATTR_PARAM_VALUE_DATA attributes (rather
>than nested) are not mentioned anywhere userspace-visible.  Would it
>make sense to add a short section to Documentation/networking/devlink/
>covering the encoding, the per-element width rules, and the maximum
>element count so that iproute2 devlink, ynl and third-party libraries
>can implement this without reading the kernel source?
DEVLINK_VAR_ATTR_TYPE_U64_ARRAY defined in include/uapi/linux/devlink.h, which is uapi.

>Does adding u64arr to this union bloat every instance of union
>devlink_param_value?
>Before this change the union was dominated by char vstr[32] so it was
>32 bytes.  With struct devlink_param_u64_array { u64 size; u64
>val[32]; } it becomes 264 bytes, and the size is paid by every union
>regardless of the param's actual type.  struct devlink_param_item
>embeds the union three times (driverinit_value, driverinit_value_new,
>driverinit_default), so every registered devlink param grows by
>roughly 700 bytes even if it is a bool or u8.
We did modify to alloc from heap to reduce size in stack frame.

>Would storing the array out-of-line (a pointer plus a length) keep the
>cost on the users of U64_ARRAY only?
I dont think it is a good design. We need to contain every thing from userspace to kernelspace
(and vice versa) in this structure itself.

>Since the in-kernel cap is 32, is there a reason for the size field to
>be u64?  A narrower type (for example u8, paired with a
>BUILD_BUG_ON(__DEVLINK_PARAM_MAX_ARRAY_SIZE > U8_MAX)) would
>structurally prevent the put path from trusting an out-of-range size.
but there is no saving as such by defining only size as u32, as each array element is
64 bit, due to alignment padding another 32bit is lost. So we made size as well 64bit.

>>  	__DEVLINK_VAR_ATTR_TYPE_CUSTOM_BASE = 0x80,
>>  	/* Any possible custom types, unrelated to NLA_* values go below */
>> +	DEVLINK_VAR_ATTR_TYPE_U64_ARRAY,
>>  };
>This is a permanent UAPI addition.  The name says U64_ARRAY but the
>kernel parser accepts either u32- or u64-encoded elements per-entry
>(nla_get_uint() in devlink_param_value_get_from_info() below).  Is
>there a reason to keep both widths accepted under a name that promises
>u64?  A single message can mix u32- and u64-encoded elements today,
>which is surprising given the type name.
This is a design suggested by maintainers, as it allows to pass both u32 and u64 size
arrays from userspace to kernel.

>Is __DEVLINK_PARAM_MAX_ARRAY_SIZE intended to be visible to userspace?
>It is defined only in include/net/devlink.h, not in this uapi header,
>not in the yaml spec, and not in Documentation/.  As a result the
>request above 32 elements is rejected without userspace having any
>way to discover the cap in advance.
I followed __DEVLINK_PARAM_MAX_STRING_VALUE, which is also defined in devlink.h.
So i dont think, we need to do it.

>> +	case DEVLINK_PARAM_TYPE_U64_ARRAY:
>> +		for (int i = 0; i < val->u64arr.size; i++)
>> +			if (nla_put_uint(msg, nla_type, val->u64arr.val[i]))
>> +				return -EMSGSIZE;
>> +		break;
>>  	}
>>  	return 0;
>>  }
>Can this loop read past val->u64arr.val[]?
>The ingress path in devlink_param_value_get_from_info() caps cnt at
>__DEVLINK_PARAM_MAX_ARRAY_SIZE (32), but this put path loops to
>val->u64arr.size with no equivalent clamp.  If a driver's ->get()
>callback returns 0 but leaves u64arr.size unset (see the kmalloc note
>on ctx below) or sets it above 32, this loop indexes past the 32-entry
>val[] and emits those bytes back to the requester over netlink.
This is okay, we can send as many u64, which can be fitted into msg (skb).
If this reads past u64arr.size, it is an issue with the driver.
Please see jiri comment- https://lore.kernel.org/netdev/3pk4hkzgwy3a55zveapgmk23bsevru55xv75vhkzbpmzkfofcx@rlnkrvynofig/