Re: [PATCH 2/3] IB/umad: Fix error handling

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Hi,

Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
> Avoid leaking a kref count in ib_umad_open() if port->ib_dev == NULL
> or if nonseekable_open() fails. Avoid leaking a kref count, that
> sm_sem is kept down and also that the IB_PORT_SM capability mask is
> not cleared in ib_umad_sm_open() if nonseekable_open() fails.
> 
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/infiniband/core/user_mad.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index e61287c..5c67d80 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -780,24 +780,20 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
>  {
>  	struct ib_umad_port *port;
>  	struct ib_umad_file *file;
> -	int ret;
> +	int ret = -ENXIO;
>  

I don't like the way ret is gratuitously set,

>  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
>  	kref_get(&port->umad_dev->ref);
>  
>  	mutex_lock(&port->file_mutex);
>  
> -	if (!port->ib_dev) {
> -		ret = -ENXIO;
> +	if (!port->ib_dev)
>  		goto out;
> -	}
>  
> +	ret = -ENOMEM;

especially here: I think it should be moved in the error handling path:

>  	file = kzalloc(sizeof *file, GFP_KERNEL);
> -	if (!file) {
> -		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> -		ret = -ENOMEM;

keep it here.

> +	if (!file)
>  		goto out;
> -	}
>  
>  	mutex_init(&file->mutex);
>  	spin_lock_init(&file->send_lock);
> @@ -814,6 +810,10 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
>  
>  out:
>  	mutex_unlock(&port->file_mutex);
> +
> +	if (ret)
> +		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> +
>  	return ret;
>  }
>  
> @@ -892,18 +892,27 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
>  	}
>  
>  	ret = ib_modify_port(port->ib_dev, port->port_num, 0, &props);
> -	if (ret) {
> -		up(&port->sm_sem);
> -		goto fail;
> -	}
> +	if (ret)
> +		goto up_sem;
>  
>  	filp->private_data = port;
>  
> -	return nonseekable_open(inode, filp);
> +	ret = nonseekable_open(inode, filp);
> +	if (ret)
> +		goto clr_sm_cap;
>  
>  fail:
> -	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> +	if (ret)
> +		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
>  	return ret;
> +
> +clr_sm_cap:
> +	swap(props.set_port_cap_mask, props.clr_port_cap_mask);
> +	ib_modify_port(port->ib_dev, port->port_num, 0, &props);
> +
> +up_sem:
> +	up(&port->sm_sem);
> +	goto fail;

I dislike jump backward, why not unconditionally call kref_put() in the
error path and return ret here.

This way you could drop fail label and the test on ret in the default
code path.

>  }
>  
>  static int ib_umad_sm_close(struct inode *inode, struct file *filp)

Regards.

-- 
Yann Droneaud
OPTEYA



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html