Re: [bug report] WARNING: possible circular locking at: rdma_destroy_id+0x17/0x20 [rdma_cm] triggered by blktests nvmeof-mp/002

Bart Van Assche
2022-05-28T19:01:01Z

On 5/27/22 14:52, Jason Gunthorpe wrote:
> On Wed, May 25, 2022 at 08:50:52PM +0200, Bart Van Assche wrote:
>> On 5/25/22 13:01, Sagi Grimberg wrote:
>>> iirc this was reported before, based on my analysis lockdep is giving
>>> a false alarm here. The reason is that the id_priv->handler_mutex cannot
>>> be the same for both cm_id that is handling the connect and the cm_id
>>> that is handling the rdma_destroy_id because rdma_destroy_id call
>>> is always called on a already disconnected cm_id, so this deadlock
>>> lockdep is complaining about cannot happen.
>>>
>>> I'm not sure how to settle this.
>>
>> If the above is correct, using lockdep_register_key() for
>> id_priv->handler_mutex instead of a static key should make the lockdep false
>> positive disappear.
>
> That only works if you can detect actual different lock classes during
> lock creation. It doesn't seem applicable in this case.

Why doesn't it seem applicable in this case? The default behavior of
mutex_init() and related initialization functions is to create one lock
class per synchronization object initialization caller.
lockdep_register_key() can be used to create one lock class per
synchronization object instance. I introduced lockdep_register_key()
myself a few years ago.

After having taken a closer look at the RDMA/CM code, I decided not yet
to implement what I proposed above. I noticed that handler_mutex is held
around callback invocations. An example:

static int cma_cm_event_handler(struct rdma_id_private *id_priv,
struct rdma_cm_event *event)
{
int ret;

lockdep_assert_held(&id_priv->handler_mutex);

trace_cm_event_handler(id_priv, event);
ret = id_priv->id.event_handler(&id_priv->id, event);
trace_cm_event_done(id_priv, event, ret);
return ret;
}

My opinion is that holding *any* lock around the invocation of a
callback function is an antipattern, in other words, something that
never should be done. John Ousterhout already described this in 1996 in
his presentation [1]. Patches like 071ba4cc559d ("RDMA: Add
rdma_connect_locked()") work around this problem but do not solve it.

Has it been considered to rework the RDMA/CM such that no locks are held
around the invocation of callback functions like the event_handler
callback? There are other mechanisms to report events from one software
layer (RDMA/CM) to a higher software layer (ULP), e.g. a linked list
with event information. The RDMA/CM could queue events onto that list
and the ULP can dequeue events from that list.

Thanks,

Bart.

[1] Ousterhout, John. "Why threads are a bad idea (for most purposes)."
In Presentation given at the 1996 Usenix Annual Technical Conference,
vol. 5. 1996.