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?
