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 <[email protected]>
> Cc: <[email protected]>
> ---
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
