Re: [PATCH 4/4] RDMA: Fix double-free in srq creation error flow

From: Jason Gunthorpe <jgg@mellanox.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>,
	Leon Romanovsky <leonro@mellanox.com>,
	RDMA mailing list <linux-rdma@vger.kernel.org>,
	Jack Morgenstein <jackm@dev.mellanox.co.il>,
	Mark Zhang <markz@mellanox.com>
Subject: Re: [PATCH 4/4] RDMA: Fix double-free in srq creation error flow
Date: Mon, 16 Sep 2019 17:57:56 +0000	[thread overview]
Message-ID: <20190916175751.GF2585@mellanox.com> (raw)
In-Reply-To: <20190916071154.20383-5-leon@kernel.org>

On Mon, Sep 16, 2019 at 10:11:54AM +0300, Leon Romanovsky wrote:
> From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> 
> The cited commit introduced a double-free of the srq buffer
> in the error flow of procedure __uverbs_create_xsrq().
> 
> The problem is that procedure ib_destroy_srq_user() called
> in the error flow also frees the srq buffer.
> 
> Thus, if uverbs_response() fails in __uverbs_create_srq(),
> the srq buffer will be freed twice.
> 
> Fixes: 68e326dea1db ("RDMA: Handle SRQ allocations by IB/core")
> Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/core/uverbs_cmd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 8f4fd4fac159..13af88da5f79 100644
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -3482,7 +3482,8 @@ static int __uverbs_create_xsrq(struct uverbs_attr_bundle *attrs,
> 
>  err_copy:
>  	ib_destroy_srq_user(srq, uverbs_get_cleared_udata(attrs));
> -
> +	/* It was released in ib_destroy_srq_user */
> +	srq = NULL;

I really don't like that we ended up with such a mess of error unwind.

The proper outcome should be that uobj_alloc_abort() does a full
clean up, including destroying and freeing the HW object if it was
allocated.

When we forced the new uobj system into the write() path this was
never cleaned up, instead the abort just disables the HW object clean
up to avoid alot of code churn, while ioctl does actually clean the HW
object on failure.

Ie only one clean up path for uobjects.

I also wonder if there is another race here, this is also missing the
ib_uverbs_release_uevent() after destroying the HW object, but I don't
know if it could be possible for an event to be stuffed in..

In any event, the double free is clearly bad, so applied to for-next

Thanks,
Jason