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@xxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
