Re: [PATCH rdma-next 1/4] RDMA/uverbs: Fix error cleanup path of ib_uverbs_add_one()

[Jason Gunthorpe <jgg@xxxxxxxx>]

On Tue, Aug 28, 2018 at 03:05:07PM +0300, Leon Romanovsky wrote:
> From: Parav Pandit <parav@xxxxxxxxxxxx>
> 
> (a) If ib_uverbs_create_uapi() fails, dev_num should be freed.
> (b) Destroy uapi if fails to create device.
> 
> Fixes: 7d96c9b17636 ("IB/uverbs: Have the core code create the uverbs_root_spec")
> Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> ---
>  drivers/infiniband/core/uverbs_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 823beca448e1..57495a93017b 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -1050,7 +1050,7 @@ static void ib_uverbs_add_one(struct ib_device *device)
>  	uverbs_dev->num_comp_vectors = device->num_comp_vectors;
>  
>  	if (ib_uverbs_create_uapi(device, uverbs_dev))
> -		goto err;
> +		goto err_uapi;
>  
>  	cdev_init(&uverbs_dev->cdev, NULL);
>  	uverbs_dev->cdev.owner = THIS_MODULE;
> @@ -1077,11 +1077,11 @@ static void ib_uverbs_add_one(struct ib_device *device)
>  
>  err_class:
>  	device_destroy(uverbs_class, uverbs_dev->cdev.dev);
> -
>  err_cdev:
> +	uverbs_destroy_api(uverbs_dev->uapi);
>  	cdev_del(&uverbs_dev->cdev);
> +err_uapi:
>  	clear_bit(devnum, dev_map);
> -
>  err:
>  	if (atomic_dec_and_test(&uverbs_dev->refcount))
>  		ib_uverbs_comp_dev(uverbs_dev);

Actually, this change to uverbs_destroy_api is wrong, so I'm not going
to apply this patch..

The control flow looks like this:

static void ib_uverbs_add_one(struct ib_device *device)
{
        kobject_init(&uverbs_dev->kobj, &ib_uverbs_dev_ktype);
        if (ib_uverbs_create_uapi(device, uverbs_dev))
           [..]
        if (cdev_add(&uverbs_dev->cdev, base, 1))
                goto err_cdev;
err_cdev:
[..]
        kobject_put(&uverbs_dev->kobj);
        return;

And ib_uverbs_release_dev() is called by kobject_put, and it does:

static void ib_uverbs_release_dev(struct kobject *kobj)
{
        uverbs_destroy_api(dev->uapi);

So, this is now double freeing uapi if we add another call to
uverbs_destroy_api.

The issue with the devnum is real though, but it is simpler to solve
it by moving the ib_uverbs_create_uapi up to before the set_bit.

Please respin the series..

Jason