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

From: Greg Kroah-Hartman
Date: Sat Apr 11 2020 - 08:21:58 EST

Next message: Greg Kroah-Hartman: "[PATCH 5.6 38/38] driver core: Reevaluate dev->links.need_for_probe as suppliers are added"
Previous message: Greg Kroah-Hartman: "[PATCH 5.6 37/38] io-uring: drop completion when removing file"
In reply to: Greg Kroah-Hartman: "[PATCH 5.6 37/38] io-uring: drop completion when removing file"
Next in thread: Greg Kroah-Hartman: "[PATCH 5.6 38/38] driver core: Reevaluate dev->links.need_for_probe as suppliers are added"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>

commit 7c11910783a1ea17e88777552ef146cace607b3c upstream.

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")
Link: https://lore.kernel.org/r/20200218210432.GA31966@xxxxxxxx
Reported-by: syzbot+adb15cf8c2798e4e0db4@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+e5579222b6a3edd96522@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+4b628fcc748474003457@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+29ee8f76017ce6cf03da@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+6956235342b7317ec564@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+b358909d8d01556b790b@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+6b46b135602a3f3ac99e@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+8458d13b13562abf6b77@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+bd034f3fdc0402e942ed@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+c92378b32760a4eef756@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+68b44a1597636e0b342c@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

---
drivers/infiniband/core/ucma.c | 49 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 2 deletions(-)

--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -91,6 +91,7 @@ struct ucma_context {

struct ucma_file *file;
struct rdma_cm_id *cm_id;
+ struct mutex mutex;
u64 uid;

struct list_head list;
@@ -216,6 +217,7 @@ static struct ucma_context *ucma_alloc_c
init_completion(&ctx->comp);
INIT_LIST_HEAD(&ctx->mc_list);
ctx->file = file;
+ mutex_init(&ctx->mutex);

if (xa_alloc(&ctx_table, &ctx->id, ctx, xa_limit_32b, GFP_KERNEL))
goto error;
@@ -589,6 +591,7 @@ static int ucma_free_ctx(struct ucma_con
}

events_reported = ctx->events_reported;
+ mutex_destroy(&ctx->mutex);
kfree(ctx);
return events_reported;
}
@@ -658,7 +661,10 @@ static ssize_t ucma_bind_ip(struct ucma_
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_bind_addr(ctx->cm_id, (struct sockaddr *) &cmd.addr);
+ mutex_unlock(&ctx->mutex);
+
ucma_put_ctx(ctx);
return ret;
}
@@ -681,7 +687,9 @@ static ssize_t ucma_bind(struct ucma_fil
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_bind_addr(ctx->cm_id, (struct sockaddr *) &cmd.addr);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -705,8 +713,10 @@ static ssize_t ucma_resolve_ip(struct uc
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_resolve_addr(ctx->cm_id, (struct sockaddr *) &cmd.src_addr,
(struct sockaddr *) &cmd.dst_addr, cmd.timeout_ms);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -731,8 +741,10 @@ static ssize_t ucma_resolve_addr(struct
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_resolve_addr(ctx->cm_id, (struct sockaddr *) &cmd.src_addr,
(struct sockaddr *) &cmd.dst_addr, cmd.timeout_ms);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -752,7 +764,9 @@ static ssize_t ucma_resolve_route(struct
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_resolve_route(ctx->cm_id, cmd.timeout_ms);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -841,6 +855,7 @@ static ssize_t ucma_query_route(struct u
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
memset(&resp, 0, sizeof resp);
addr = (struct sockaddr *) &ctx->cm_id->route.addr.src_addr;
memcpy(&resp.src_addr, addr, addr->sa_family == AF_INET ?
@@ -864,6 +879,7 @@ static ssize_t ucma_query_route(struct u
ucma_copy_iw_route(&resp, &ctx->cm_id->route);

out:
+ mutex_unlock(&ctx->mutex);
if (copy_to_user(u64_to_user_ptr(cmd.response),
&resp, sizeof(resp)))
ret = -EFAULT;
@@ -1014,6 +1030,7 @@ static ssize_t ucma_query(struct ucma_fi
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
switch (cmd.option) {
case RDMA_USER_CM_QUERY_ADDR:
ret = ucma_query_addr(ctx, response, out_len);
@@ -1028,6 +1045,7 @@ static ssize_t ucma_query(struct ucma_fi
ret = -ENOSYS;
break;
}
+ mutex_unlock(&ctx->mutex);

ucma_put_ctx(ctx);
return ret;
@@ -1068,7 +1086,9 @@ static ssize_t ucma_connect(struct ucma_
return PTR_ERR(ctx);

ucma_copy_conn_param(ctx->cm_id, &conn_param, &cmd.conn_param);
+ mutex_lock(&ctx->mutex);
ret = rdma_connect(ctx->cm_id, &conn_param);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -1089,7 +1109,9 @@ static ssize_t ucma_listen(struct ucma_f

ctx->backlog = cmd.backlog > 0 && cmd.backlog < max_backlog ?
cmd.backlog : max_backlog;
+ mutex_lock(&ctx->mutex);
ret = rdma_listen(ctx->cm_id, ctx->backlog);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -1112,13 +1134,17 @@ static ssize_t ucma_accept(struct ucma_f
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);
- } else
+ } else {
+ mutex_lock(&ctx->mutex);
ret = __rdma_accept(ctx->cm_id, NULL, NULL);
-
+ mutex_unlock(&ctx->mutex);
+ }
ucma_put_ctx(ctx);
return ret;
}
@@ -1137,7 +1163,9 @@ static ssize_t ucma_reject(struct ucma_f
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_reject(ctx->cm_id, cmd.private_data, cmd.private_data_len);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -1156,7 +1184,9 @@ static ssize_t ucma_disconnect(struct uc
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
ret = rdma_disconnect(ctx->cm_id);
+ mutex_unlock(&ctx->mutex);
ucma_put_ctx(ctx);
return ret;
}
@@ -1187,7 +1217,9 @@ static ssize_t ucma_init_qp_attr(struct
resp.qp_attr_mask = 0;
memset(&qp_attr, 0, sizeof qp_attr);
qp_attr.qp_state = cmd.qp_state;
+ mutex_lock(&ctx->mutex);
ret = rdma_init_qp_attr(ctx->cm_id, &qp_attr, &resp.qp_attr_mask);
+ mutex_unlock(&ctx->mutex);
if (ret)
goto out;

@@ -1273,9 +1305,13 @@ static int ucma_set_ib_path(struct ucma_
struct sa_path_rec opa;

sa_convert_path_ib_to_opa(&opa, &sa_path);
+ mutex_lock(&ctx->mutex);
ret = rdma_set_ib_path(ctx->cm_id, &opa);
+ mutex_unlock(&ctx->mutex);
} else {
+ mutex_lock(&ctx->mutex);
ret = rdma_set_ib_path(ctx->cm_id, &sa_path);
+ mutex_unlock(&ctx->mutex);
}
if (ret)
return ret;
@@ -1308,7 +1344,9 @@ static int ucma_set_option_level(struct

switch (level) {
case RDMA_OPTION_ID:
+ mutex_lock(&ctx->mutex);
ret = ucma_set_option_id(ctx, optname, optval, optlen);
+ mutex_unlock(&ctx->mutex);
break;
case RDMA_OPTION_IB:
ret = ucma_set_option_ib(ctx, optname, optval, optlen);
@@ -1368,8 +1406,10 @@ static ssize_t ucma_notify(struct ucma_f
if (IS_ERR(ctx))
return PTR_ERR(ctx);

+ mutex_lock(&ctx->mutex);
if (ctx->cm_id->device)
ret = rdma_notify(ctx->cm_id, (enum ib_event_type)cmd.event);
+ mutex_unlock(&ctx->mutex);

ucma_put_ctx(ctx);
return ret;
@@ -1412,8 +1452,10 @@ static ssize_t ucma_process_join(struct
mc->join_state = join_state;
mc->uid = cmd->uid;
memcpy(&mc->addr, addr, cmd->addr_size);
+ mutex_lock(&ctx->mutex);
ret = rdma_join_multicast(ctx->cm_id, (struct sockaddr *)&mc->addr,
join_state, mc);
+ mutex_unlock(&ctx->mutex);
if (ret)
goto err2;

@@ -1513,7 +1555,10 @@ static ssize_t ucma_leave_multicast(stru
goto out;
}

+ mutex_lock(&mc->ctx->mutex);
rdma_leave_multicast(mc->ctx->cm_id, (struct sockaddr *) &mc->addr);
+ mutex_unlock(&mc->ctx->mutex);
+
mutex_lock(&mc->ctx->file->mut);
ucma_cleanup_mc_events(mc);
list_del(&mc->list);

Next message: Greg Kroah-Hartman: "[PATCH 5.6 38/38] driver core: Reevaluate dev->links.need_for_probe as suppliers are added"
Previous message: Greg Kroah-Hartman: "[PATCH 5.6 37/38] io-uring: drop completion when removing file"
In reply to: Greg Kroah-Hartman: "[PATCH 5.6 37/38] io-uring: drop completion when removing file"
Next in thread: Greg Kroah-Hartman: "[PATCH 5.6 38/38] driver core: Reevaluate dev->links.need_for_probe as suppliers are added"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]