Re: [PATCH net-next V9 07/14] devlink: Allow rate node parents from other devlinks

[Cosmin Ratiu <cratiu@nvidia.com>]

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: Allow rate node parents from other devlinks
> 
> This commit makes use of the building blocks previously added to
> implement cross-device rate nodes.
> 
> > diff --git a/include/net/devlink.h b/include/net/devlink.h
> > index 3038af6ec017e..8d5ad5d4f1d05 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -1585,6 +1585,11 @@ struct devlink_ops {
> >  				    struct devlink_rate *parent,
> >  				    void *priv_child, void
> > *priv_parent,
> >  				    struct netlink_ext_ack
> > *extack);
> > +	/* Indicates if cross-device rate nodes are supported.
> > +	 * This also requires a shared common ancestor object all
> > devices that
> > +	 * could share rate nodes are nested in.
> > +	 */
> > +	bool supported_cross_device_rate_nodes;
> 
> When a device acting as a cross-device rate parent is destroyed,
> devl_rate_nodes_destroy() iterates the shared rate_list to detach its
> children. If it encounters a child rate node belonging to a different
> device,
> it invokes the child driver's ops->rate_node_parent_set or
> ops->rate_leaf_parent_set callbacks.
> 
> Since the cleanup path only holds the parent's devlink lock and the
> shared
> root devlink lock, does this allow the child driver's rate operations
> to
> execute concurrently with other operations on the child device
> without holding
> the child's devlink instance lock?

The rate_node lock protects changes to the rate hierarchy, even if
child devlink instance locks aren't held. The code works by locking
everything from the affected instance A up to the instance at the root
of the hierarchy R, and affects the hierarchy with R's instance lock
held. I think this can be made clearer in the code/comments though, so
Will tweak some things in the next version.

> 
> [ ... ]
> > diff --git a/net/devlink/rate.c b/net/devlink/rate.c
> > index 1949746fab291..f243cccc95bee 100644
> > --- a/net/devlink/rate.c
> > +++ b/net/devlink/rate.c
> > @@ -30,19 +30,53 @@ devlink_rate_leaf_get_from_info(struct devlink
> > *devlink, struct genl_info *info)
> >  	return devlink_rate ?: ERR_PTR(-ENODEV);
> >  }
> >  
> > +/* Repeatedly locks the nested-in devlink instances while cross
> > device rate
> > + * nodes are supported. Returns the devlink instance where rates
> > should be
> > + * stored.
> > + */
> >  static struct devlink *devl_rate_lock(struct devlink *devlink)
> >  {
> > -	return devlink;
> > +	struct devlink *rate_devlink = devlink;
> > +
> > +	while (rate_devlink->ops &&
> > +	       rate_devlink->ops-
> > >supported_cross_device_rate_nodes) {
> > +		devlink = devlink_nested_in_get_lock(rate_devlink-
> > >rel);
> > +		if (!devlink)
> > +			break;
> > +		rate_devlink = devlink;
> > +	}
> > +	return rate_devlink;
> >  }
> 
> [ ... ]
> 
> > +/* Repeatedly unlocks the nested-in devlink instances of 'devlink'
> > while cross
> > + * device nodes are supported.
> > + */
> >  static void devl_rate_unlock(struct devlink *devlink)
> >  {
> > +	if (!devlink || !devlink->ops ||
> > +	    !devlink->ops->supported_cross_device_rate_nodes)
> > +		return;
> > +
> > +	devl_rate_unlock(devlink_nested_in_get_locked(devlink-
> > >rel));
> > +	devlink_nested_in_put_unlock(devlink->rel);
> >  }
> 
> If a parent devlink is not yet registered,
> devlink_nested_in_get_lock() can
> return NULL in devl_rate_lock() and break the loop without taking a
> lock.
> 
> If another thread completes registering the parent devlink
> concurrently,
> devlink_nested_in_get_locked() in devl_rate_unlock() will find it. 
> 
> Could this lead to unlocking a mutex that was never acquired and
> dropping a
> reference that was never taken, resulting in a use-after-free or lock
> corruption?

A valid concern in theory, even though in practice there won't be
another thread registering a shared devlink instance that would race
with registration in this way.

I will make these lock/unlock helpers more robust in the next version.

> 
> [ ... ]
> > @@ -320,13 +371,14 @@ devlink_nl_rate_parent_node_set(struct
> > devlink_rate *devlink_rate,
> >  				struct genl_info *info,
> >  				struct nlattr *nla_parent)
> >  {
> > -	struct devlink *devlink = devlink_rate->devlink;
> > +	struct devlink *devlink = devlink_rate->devlink,
> > *parent_devlink;
> >  	const char *parent_name = nla_data(nla_parent);
> >  	const struct devlink_ops *ops = devlink->ops;
> >  	size_t len = strlen(parent_name);
> >  	struct devlink_rate *parent;
> >  	int err = -EOPNOTSUPP;
> >  
> > +	parent_devlink = devlink_nl_ctx(info)->parent_devlink ? :
> > devlink;
> >  	parent = devlink_rate->parent;
> >  
> >  	if (parent && !len) {
> > @@ -344,7 +396,13 @@ devlink_nl_rate_parent_node_set(struct
> > devlink_rate *devlink_rate,
> >  		refcount_dec(&parent->refcnt);
> >  		devlink_rate->parent = NULL;
> >  	} else if (len) {
> > -		parent = devlink_rate_node_get_by_name(devlink,
> > parent_name);
> > +		/* parent_devlink (when different than devlink)
> > isn't locked,
> > +		 * but the rate node devlink instance is, so
> > nobody from the
> > +		 * same group of devices sharing rates could
> > change the used
> > +		 * fields or unregister the parent.
> > +		 */
> > +		parent =
> > devlink_rate_node_get_by_name(parent_devlink,
> > +						      
> > parent_name);
> 
> Is parent_devlink validated to ensure it shares the same common rate
> root
> ancestor as devlink?
> 
> If an arbitrary unrelated parent_devlink is specified,
> devlink_rate_node_get_by_name() could traverse an unrelated parent's
> rate_list that was never locked by devl_rate_lock(), leading to a
> data race.
> 
> Additionally, if a rate node is found, its priv pointer is passed to
> the
> target driver's ops->rate_node_parent_set. Could this cause the
> target driver
> to cast a foreign priv pointer to its own private struct type,
> resulting in
> type confusion and memory corruption?

Right, there's no common ancestor validation which could lead to
unpleasant results if one tries to set a rate parent across drivers
which support cross-esw.
I'll add it in the next version.

Cosmin.