| Message ID | 20180404030741.GA32035@ziepe.ca (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Apr 03, 2018 at 09:07:41PM -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 | 34 ++++++++++++++++++++++------------ > 1 file changed, 22 insertions(+), 12 deletions(-) > > Inspired by Roland's patch, let us try hard to be sure we are done with > this issue. > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > index e74c82ee38a9f7..95e1eadae074ff 100644 > --- a/drivers/infiniband/core/ucma.c > +++ b/drivers/infiniband/core/ucma.c > @@ -153,6 +153,21 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id) > return ctx; > } > > +/* > + * 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) > + return ERR_PTR(-EINVAL); > + return ctx; > +} I like the idea, but the implementation needs to be improved a little bit. All old callers to ucma_get_ctx() expected that once that call fails they won't need to release "ctx". Now, the failures returned by ucma_get_ctx_dev() break that flow. The function should look like this: +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; +} Thanks
On Wed, Apr 04, 2018 at 09:08:56AM +0300, Leon Romanovsky wrote: > On Tue, Apr 03, 2018 at 09:07:41PM -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 | 34 ++++++++++++++++++++++------------ > > 1 file changed, 22 insertions(+), 12 deletions(-) > > > > Inspired by Roland's patch, let us try hard to be sure we are done with > > this issue. > > > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > > index e74c82ee38a9f7..95e1eadae074ff 100644 > > +++ b/drivers/infiniband/core/ucma.c > > @@ -153,6 +153,21 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id) > > return ctx; > > } > > > > +/* > > + * 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) > > + return ERR_PTR(-EINVAL); > > + return ctx; > > +} > > I like the idea, but the implementation needs to be improved a little bit. > All old callers to ucma_get_ctx() expected that once that call fails > they won't need to release "ctx". Now, the failures returned by ucma_get_ctx_dev() > break that flow. Oops, yes, thanks. v2 on its way 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
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index e74c82ee38a9f7..95e1eadae074ff 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -153,6 +153,21 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id) return ctx; } +/* + * 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) + return ERR_PTR(-EINVAL); + return ctx; +} + static void ucma_put_ctx(struct ucma_context *ctx) { if (atomic_dec_and_test(&ctx->ref)) @@ -734,7 +749,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 +1065,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 +1107,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 +1135,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 +1154,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 +1182,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 +1391,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 | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) Inspired by Roland's patch, let us try hard to be sure we are done with this issue.