Re: [PATCH RFC 1/2] RDMA/cma: Add rdma_reclassify_id_for_listen()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] RDMA/cma: missing handler_mutex lockdep reclassification in
  cma_listen_on_dev()
- [Low] RDMA/cma: missing kernel-doc for rdma_reclassify_id_for_listen()
  ordering rules
--

commit 1c34d3d5b4de4179de0a92ec2b97020a760e60b6
Author: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

RDMA/cma: Add rdma_reclassify_id_for_listen()

This commit adds rdma_reclassify_id_for_listen() to assign a separate lockdep
class to the handler_mutex of listening rdma_cm_id instances.

Link: https://lore.kernel.org/linux-nvme/at36um4zrsfOr3kh@shinmob/

> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 337a49d1acf72..436e2467a36ff 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -1064,6 +1064,19 @@ __rdma_create_kernel_id(struct net *net, rdma_cm_event_handler event_handler,
>  }
>  EXPORT_SYMBOL(__rdma_create_kernel_id);
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +static struct lock_class_key cma_listen_handler_mutex_key;
> +void rdma_reclassify_id_for_listen(struct rdma_cm_id *id)
> +{
> +	struct rdma_id_private *id_priv;
> +
> +	id_priv = container_of(id, struct rdma_id_private, id);
> +	lockdep_set_class(&id_priv->handler_mutex,
> +			  &cma_listen_handler_mutex_key);
> +}
> +EXPORT_SYMBOL(rdma_reclassify_id_for_listen);
> +#endif

[Severity: Medium]
Does cma_listen_on_dev() also need to reclassify dev_id_priv->handler_mutex
for wildcard listeners?

When a caller binds a listening cm_id to a wildcard address (such as 0.0.0.0
or ::), rdma_listen() -> cma_listen_on_all() -> cma_listen_on_dev() allocates
an internal per-device child listening ID (dev_id_priv) for each RDMA device:

drivers/infiniband/core/cma.c:cma_listen_on_dev() {
	...
	dev_id_priv =
		__rdma_create_id(net, cma_listen_handler, id_priv,
				 id_priv->id.ps, id_priv->id.qp_type, id_priv);
	...
}

Because dev_id_priv->handler_mutex keeps the default handler_mutex lockdep
class, destroying the wildcard listening cm_id under nvmet_config_sem still
triggers the same lockdep circular locking dependency warning when
rdma_destroy_id() -> _destroy_id() -> cma_cancel_operation() ->
cma_cancel_listens() -> _cma_cancel_listens() calls
rdma_destroy_id(&dev_id_priv->id).

> diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
> index d639ff889e649..59dda4b0173e7 100644
> --- a/include/rdma/rdma_cm.h
> +++ b/include/rdma/rdma_cm.h
> @@ -158,6 +158,12 @@ struct rdma_cm_id *rdma_create_user_id(rdma_cm_event_handler event_handler,
>  	__rdma_create_kernel_id(net, event_handler, context, ps, qp_type,      \
>  				KBUILD_MODNAME)
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +void rdma_reclassify_id_for_listen(struct rdma_cm_id *id);
> +#else
> +static inline void rdma_reclassify_id_for_listen(struct rdma_cm_id *id) {}
> +#endif

[Severity: Low]
Could rdma_reclassify_id_for_listen() have a kernel-doc comment like the
neighboring declarations in include/rdma/rdma_cm.h?  Since it calls
lockdep_set_class() on handler_mutex, documenting that it must be called on a
newly created rdma_cm_id before rdma_bind_addr() or rdma_listen() can acquire
handler_mutex would make the ordering requirement clear to callers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108120154.148824-1-shinichiro.kawasaki@wdc.com?part=1