On Mon, 2026-03-30 at 19:08 -0700, Jakub Kicinski wrote:
> 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?
This is a preexisting bug caused by the fact that devlink_nl_rate_set
doesn't rollback what it did on failure, leaving various side effects.
Most are setting of various rate params, but this parent refcount is
more serious. It will have to be addressed, but I don't think this
series is the right place for it. (I'll send a separate patch soon).
>
> [ ... ]
> > @@ -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?
Another preexisting issue. These code changes brought this section to
Argus' Eyes attention. I made a note to send a patch to fix this.
Cosmin.
