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.
