Re: [PATCH net-next V9 04/14] devlink: Decouple rate storage from associated devlink object

[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.
---
devlink: Decouple rate storage from associated devlink object

Devlink rate leafs and nodes were stored in their respective devlink
objects pointed to by devlink_rate->devlink.

> diff --git a/net/devlink/rate.c b/net/devlink/rate.c
> index 4781429109195..9ebbc72130c6a 100644
> --- a/net/devlink/rate.c
> +++ b/net/devlink/rate.c
[ ... ]
> @@ -648,8 +702,9 @@ int devlink_nl_rate_new_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto err_rate_set;
>  
>  	refcount_set(&rate_node->refcnt, 1);
> -	list_add(&rate_node->list, &devlink->rate_list);
> +	list_add(&rate_node->list, &rate_devlink->rate_list);
>  	devlink_rate_notify(rate_node, DEVLINK_CMD_RATE_NEW);
> +	devl_rate_unlock(devlink);
>  	return 0;
>  
>  err_rate_set:
>  	ops->rate_node_del(rate_node, rate_node->priv, info->extack);
> @@ -658,6 +713,8 @@ int devlink_nl_rate_new_doit(struct sk_buff *skb, struct genl_info *info)
>  	kfree(rate_node->name);
>  err_strdup:
>  	kfree(rate_node);
> +unlock:
> +	devl_rate_unlock(devlink);
>  	return err;
>  }

If devlink_nl_rate_set() fails after it successfully sets the parent node
and increments the parent's refcount, does this error path permanently
leak that reference since it frees the node without decrementing it?

[ ... ]
> @@ -716,14 +785,20 @@ devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
>  		      struct devlink_rate *parent)
>  {
>  	struct devlink_rate *rate_node;
> +	struct devlink *rate_devlink;
>  
> +	rate_devlink = devl_rate_lock(devlink);
>  	rate_node = devlink_rate_node_get_by_name(devlink, node_name);
> -	if (!IS_ERR(rate_node))
> -		return ERR_PTR(-EEXIST);
> +	if (!IS_ERR(rate_node)) {
> +		rate_node = ERR_PTR(-EEXIST);
> +		goto unlock;
> +	}
>  
>  	rate_node = kzalloc_obj(*rate_node);
> -	if (!rate_node)
> -		return ERR_PTR(-ENOMEM);
> +	if (!rate_node) {
> +		rate_node = ERR_PTR(-ENOMEM);
> +		goto unlock;
> +	}
>  
>  	if (parent) {
>  		rate_node->parent = parent;
> @@ -737,12 +812,15 @@ devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
>  	rate_node->name = kstrdup(node_name, GFP_KERNEL);
>  	if (!rate_node->name) {
>  		kfree(rate_node);
> -		return ERR_PTR(-ENOMEM);
> +		rate_node = ERR_PTR(-ENOMEM);
> +		goto unlock;
>  	}

Similarly, if kstrdup() fails here, does this leak the parent's refcount
since it was already incremented earlier in the function?