Re: [PATCH] RDMA/ucma: Put a lock around every call to the rdma_cm layer

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Feb 18, 2020 at 09:04:36PM +0000, Jason Gunthorpe wrote:
> The rdma_cm must be used single threaded.
> 
> This appears to be a bug in the design, as it does have lots of locking
> that seems like it should allow concurrency. However, when it is all said
> and done every single place that uses the cma_exch() scheme is broken, and
> all the unlocked reads from the ucma of the cm_id data are wrong too.
> 
> syzkaller has been finding endless bugs related to this.
> 
> Fixing this in any elegant way is some enormous amount of work. Take a
> very big hammer and put a mutex around everything to do with the
> ucma_context at the top of every syscall.
> 
> Fixes: 75216638572f ("RDMA/cma: Export rdma cm interface to userspace")
> Reported-by: syzbot+adb15cf8c2798e4e0db4@syzkaller.appspotmail.com
> Reported-by: syzbot+e5579222b6a3edd96522@syzkaller.appspotmail.com
> Reported-by: syzbot+4b628fcc748474003457@syzkaller.appspotmail.com
> Reported-by: syzbot+29ee8f76017ce6cf03da@syzkaller.appspotmail.com
> Reported-by: syzbot+6956235342b7317ec564@syzkaller.appspotmail.com
> Reported-by: syzbot+b358909d8d01556b790b@syzkaller.appspotmail.com
> Reported-by: syzbot+6b46b135602a3f3ac99e@syzkaller.appspotmail.com
> Reported-by: syzbot+8458d13b13562abf6b77@syzkaller.appspotmail.com
> Reported-by: syzbot+bd034f3fdc0402e942ed@syzkaller.appspotmail.com
> Reported-by: syzbot+c92378b32760a4eef756@syzkaller.appspotmail.com
> Reported-by: syzbot+68b44a1597636e0b342c@syzkaller.appspotmail.com
> Cc: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  drivers/infiniband/core/ucma.c | 50 ++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
> 
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-rc
> 
> Lets see if I told syzkaller about this properly..
> 
> EricB: If there are other rdma_cm related hits in syzkaller besides
> these 11 lets include them as  well. I wasn't able to find a way to
> search for things, this list is from your past email, thanks.
> 

Unfortunately I haven't had time to work on syzkaller bugs lately, so I can't
provide an updated list until I go through the long backlog of bugs.

A comment on the patch below:

> @@ -1112,13 +1134,17 @@ static ssize_t ucma_accept(struct ucma_file *file, const char __user *inbuf,
>  	if (cmd.conn_param.valid) {
>  		ucma_copy_conn_param(ctx->cm_id, &conn_param, &cmd.conn_param);
>  		mutex_lock(&file->mut);
> +		mutex_lock(&ctx->mutex);
>  		ret = __rdma_accept(ctx->cm_id, &conn_param, NULL);
> +		mutex_unlock(&ctx->mutex);
>  		if (!ret)
>  			ctx->uid = cmd.uid;
>  		mutex_unlock(&file->mut);

This is nesting the new ucma_context::mutex inside the existing ucma_file::mut.

> @@ -1403,6 +1443,7 @@ static ssize_t ucma_process_join(struct ucma_file *file,
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  
> +	mutex_lock(&ctx->mutex);
>  	mutex_lock(&file->mut);
>  	mc = ucma_alloc_multicast(ctx);
>  	if (!mc) {

... but this is doing the opposite.  So it can deadlock.

What's the intended order?

Also, are these two separate mutexes actually needed?  I.e., did you consider
using the existing ucma_file::mut, but it didn't work or it wasn't fine-grained
enough?  (It looks like one ucma_file can have multiple ucma_contexts.)

- Eric