Re: [net-next,07/10] devlink: allow devlink instance allocation without a backing device

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "FYI"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
devlink: allow devlink instance allocation without a backing device

Allow devlink_alloc_ns() to be called with dev=NULL to support
device-less devlink instances. When dev is NULL, the instance is
identified over netlink using "devlink_index" as bus_name and
the decimal index value as dev_name.

> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 1ba3436db4ae..7de2d8cc862f 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -19,6 +19,8 @@
>  #define DEVLINK_GENL_VERSION 0x1
>  #define DEVLINK_GENL_MCGRP_CONFIG_NAME "config"
>
> +#define DEVLINK_INDEX_BUS_NAME "devlink_index"
> +
>  enum devlink_command {
>  	/* don't change the order or add anything between, this is ABI! */
>  	DEVLINK_CMD_UNSPEC,
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index 85e83a3b082a..9cdda570e584 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c

[ ... ]

> @@ -445,7 +448,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  	static u32 last_id;
>  	int ret;
>
> -	WARN_ON(!ops || !dev);
> +	WARN_ON(!ops);

The kdoc comment for devlink_alloc_ns() describes @dev as 'parent device'
but doesn't document that dev can now be NULL to create a device-less
devlink instance. Could the kdoc be updated to describe the new behavior
when dev is NULL?

>  	if (!devlink_reload_actions_valid(ops))
>  		return NULL;
>
> @@ -453,14 +456,22 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  	if (!devlink)
>  		return NULL;
>
> +	if (dev) {
> +		devlink->dev = get_device(dev);
> +		devlink->bus_name = dev->bus->name;
> +		devlink->dev_name = dev_name(dev);
> +	} else {
> +		devlink->bus_name = DEVLINK_INDEX_BUS_NAME;
> +		devlink->dev_name = kasprintf(GFP_KERNEL, "%u", devlink->index);
> +		if (!devlink->dev_name)
> +			goto err_kasprintf;
> +	}
> +
>  	ret = xa_alloc_cyclic(&devlinks, &devlink->index, devlink, xa_limit_31b,
>  			      &last_id, GFP_KERNEL);
>  	if (ret < 0)
>  		goto err_xa_alloc;

Does the kasprintf() call format devlink->index correctly? The devlink
struct is allocated with kvzalloc() which zero-fills it, so devlink->index
is still 0 when kasprintf() reads it. The xa_alloc_cyclic() that assigns
the real index happens afterward.

This means dev_name will always be "0" for device-less devlink instances,
regardless of the actual index assigned by xa_alloc_cyclic(). This breaks
the index-based lookup in devlink_get_from_attrs_lock(), which parses
dev_name back to an index with kstrtoul() and looks it up in the xarray.

The commit message states the instance should be "identified over netlink
using devlink_index as bus_name and the decimal index value as dev_name",
but with dev_name always being "0", all device-less instances would have
the same identifier.

Should the kasprintf() call be moved after xa_alloc_cyclic() succeeds?

>
> -	devlink->dev = get_device(dev);
> -	devlink->bus_name = dev->bus->name;
> -	devlink->dev_name = dev_name(dev);
>  	devlink->ops = ops;

[ ... ]

> @@ -486,6 +497,11 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  	return devlink;
>
>  err_xa_alloc:
> +	if (devlink->dev)
> +		put_device(devlink->dev);
> +	else
> +		kfree(devlink->dev_name);
> +err_kasprintf:
>  	kvfree(devlink);
>  	return NULL;
>  }

[ ... ]