| Message ID | 20180405030001.GA23127@ziepe.ca (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Apr 04, 2018 at 09:00:01PM -0600, Jason Gunthorpe wrote: > This is done by auditing all callers of ucma_get_ctx and switching the > ones that unconditionally touch ->device to ucma_get_ctx_dev. This covers > a little less than half of the call sites. > > The 11 remaining call sites to ucma_get_ctx() were manually audited. > > It looks like none of these cases cause bugs due to the FSM system > inside the CMA, but documenting the requirement and the result of this > audit is still productive. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- > drivers/infiniband/core/ucma.c | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) > > v3: > - Reorder the function after put_ctx so it compiles warning free > I really shouldn't rush so much :( > Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Thu, 2018-04-05 at 18:55 +0300, Leon Romanovsky wrote: > On Wed, Apr 04, 2018 at 09:00:01PM -0600, Jason Gunthorpe wrote: > > This is done by auditing all callers of ucma_get_ctx and switching the > > ones that unconditionally touch ->device to ucma_get_ctx_dev. This covers > > a little less than half of the call sites. > > > > The 11 remaining call sites to ucma_get_ctx() were manually audited. > > > > It looks like none of these cases cause bugs due to the FSM system > > inside the CMA, but documenting the requirement and the result of this > > audit is still productive. > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > --- > > drivers/infiniband/core/ucma.c | 36 ++++++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > v3: > > - Reorder the function after put_ctx so it compiles warning free > > I really shouldn't rush so much :( > > > > Thanks, > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> Thanks, applied. This seemed important enough to warrant early -rc, so I put it there instead of for-next.
On Thu, Apr 19, 2018 at 02:23:25PM -0400, Doug Ledford wrote: > On Thu, 2018-04-05 at 18:55 +0300, Leon Romanovsky wrote: > > On Wed, Apr 04, 2018 at 09:00:01PM -0600, Jason Gunthorpe wrote: > > > This is done by auditing all callers of ucma_get_ctx and switching the > > > ones that unconditionally touch ->device to ucma_get_ctx_dev. This covers > > > a little less than half of the call sites. > > > > > > The 11 remaining call sites to ucma_get_ctx() were manually audited. > > > > > > It looks like none of these cases cause bugs due to the FSM system > > > inside the CMA, but documenting the requirement and the result of this > > > audit is still productive. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > drivers/infiniband/core/ucma.c | 36 ++++++++++++++++++++++++------------ > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > v3: > > > - Reorder the function after put_ctx so it compiles warning free > > > I really shouldn't rush so much :( > > > > > > > Thanks, > > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > > Thanks, applied. This seemed important enough to warrant early -rc, so > I put it there instead of for-next. You may want to change the last paragraph there as it apparently does fix syzkaller bugs :( If I'd known that it would have gone weeks ago. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2018-04-19 at 12:49 -0600, Jason Gunthorpe wrote: > On Thu, Apr 19, 2018 at 02:23:25PM -0400, Doug Ledford wrote: > > On Thu, 2018-04-05 at 18:55 +0300, Leon Romanovsky wrote: > > > On Wed, Apr 04, 2018 at 09:00:01PM -0600, Jason Gunthorpe wrote: > > > > This is done by auditing all callers of ucma_get_ctx and switching the > > > > ones that unconditionally touch ->device to ucma_get_ctx_dev. This covers > > > > a little less than half of the call sites. > > > > > > > > The 11 remaining call sites to ucma_get_ctx() were manually audited. > > > > > > > > It looks like none of these cases cause bugs due to the FSM system > > > > inside the CMA, but documenting the requirement and the result of this > > > > audit is still productive. > > > > > > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > > drivers/infiniband/core/ucma.c | 36 ++++++++++++++++++++++++------------ > > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > > > v3: > > > > - Reorder the function after put_ctx so it compiles warning free > > > > I really shouldn't rush so much :( > > > > > > > > > > Thanks, > > > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > > > > Thanks, applied. This seemed important enough to warrant early -rc, so > > I put it there instead of for-next. > > You may want to change the last paragraph there as it apparently does > fix syzkaller bugs :( I can still do that, I haven't pushed my tree for today as of yet. I'll fix it up before I push. > If I'd known that it would have gone weeks ago. I wondered about that ;-)
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index e74c82ee38a9f7..c838b291bbb72a 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -159,6 +159,23 @@ static void ucma_put_ctx(struct ucma_context *ctx) complete(&ctx->comp); } +/* + * Same as ucm_get_ctx but requires that ->cm_id->device is valid, eg that the + * CM_ID is bound. + */ +static struct ucma_context *ucma_get_ctx_dev(struct ucma_file *file, int id) +{ + struct ucma_context *ctx = ucma_get_ctx(file, id); + + if (IS_ERR(ctx)) + return ctx; + if (!ctx->cm_id->device) { + ucma_put_ctx(ctx); + return ERR_PTR(-EINVAL); + } + return ctx; +} + static void ucma_close_event_id(struct work_struct *work) { struct ucma_event *uevent_close = container_of(work, struct ucma_event, close_work); @@ -734,7 +751,7 @@ static ssize_t ucma_resolve_route(struct ucma_file *file, if (copy_from_user(&cmd, inbuf, sizeof(cmd))) return -EFAULT; - ctx = ucma_get_ctx(file, cmd.id); + ctx = ucma_get_ctx_dev(file, cmd.id); if (IS_ERR(ctx)) return PTR_ERR(ctx); @@ -1050,7 +1067,7 @@ static ssize_t ucma_connect(struct ucma_file *file, const char __user *inbuf, if (!cmd.conn_param.valid) return -EINVAL; - ctx = ucma_get_ctx(file, cmd.id); + ctx = ucma_get_ctx_dev(file, cmd.id); if (IS_ERR(ctx)) return PTR_ERR(ctx); @@ -1092,7 +1109,7 @@ static ssize_t ucma_accept(struct ucma_file *file, const char __user *inbuf, if (copy_from_user(&cmd, inbuf, sizeof(cmd))) return -EFAULT; - ctx = ucma_get_ctx(file, cmd.id); + ctx = ucma_get_ctx_dev(file, cmd.id); if (IS_ERR(ctx)) return PTR_ERR(ctx); @@ -1120,7 +1137,7 @@ static ssize_t ucma_reject(struct ucma_file *file, const char __user *inbuf, if (copy_from_user(&cmd, inbuf, sizeof(cmd))) return -EFAULT; - ctx = ucma_get_ctx(file, cmd.id); + ctx = ucma_get_ctx_dev(file, cmd.id); if (IS_ERR(ctx)) return PTR_ERR(ctx); @@ -1139,7 +1156,7 @@ static ssize_t ucma_disconnect(struct ucma_file *file, const char __user *inbuf, if (copy_from_user(&cmd, inbuf, sizeof(cmd))) return -EFAULT; - ctx = ucma_get_ctx(file, cmd.id); + ctx = ucma_get_ctx_dev(file, cmd.id); if (IS_ERR(ctx)) return PTR_ERR(ctx); @@ -1167,15 +1184,10 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, if (cmd.qp_state > IB_QPS_ERR) return -EINVAL; - ctx = ucma_get_ctx(file, cmd.id); + ctx = ucma_get_ctx_dev(file, cmd.id); if (IS_ERR(ctx)) return PTR_ERR(ctx); - if (!ctx->cm_id->device) { - ret = -EINVAL; - goto out; - } - resp.qp_attr_mask = 0; memset(&qp_attr, 0, sizeof qp_attr); qp_attr.qp_state = cmd.qp_state; @@ -1381,7 +1393,7 @@ static ssize_t ucma_process_join(struct ucma_file *file, else return -EINVAL; - ctx = ucma_get_ctx(file, cmd->id); + ctx = ucma_get_ctx_dev(file, cmd->id); if (IS_ERR(ctx)) return PTR_ERR(ctx);
This is done by auditing all callers of ucma_get_ctx and switching the ones that unconditionally touch ->device to ucma_get_ctx_dev. This covers a little less than half of the call sites. The 11 remaining call sites to ucma_get_ctx() were manually audited. It looks like none of these cases cause bugs due to the FSM system inside the CMA, but documenting the requirement and the result of this audit is still productive. Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> --- drivers/infiniband/core/ucma.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) v3: - Reorder the function after put_ctx so it compiles warning free I really shouldn't rush so much :(