| Message ID | 537086BA.3020807@acm.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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@acm.org> > Cc: <stable@vger.kernel.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.
On 05/12/14 12:18, Yann Droneaud wrote: > Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit : >> 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. Does this mean that you are not aware that setting the return code before an if-test is a common coding style in the Linux kernel ? See e.g. kernel/futex.c or kernel/events/core.c for other examples. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le lundi 12 mai 2014 à 12:32 +0200, Bart Van Assche a écrit : > On 05/12/14 12:18, Yann Droneaud wrote: > > Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit : > >> 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. > > Does this mean that you are not aware that setting the return code > before an if-test is a common coding style in the Linux kernel ? See > e.g. kernel/futex.c or kernel/events/core.c for other examples. > Perhaps, but it's nowhere else in user_mad.c Regards.
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; 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; file = kzalloc(sizeof *file, GFP_KERNEL); - if (!file) { - kref_put(&port->umad_dev->ref, ib_umad_release_dev); - ret = -ENOMEM; + 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; } static int ib_umad_sm_close(struct inode *inode, struct file *filp)
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@acm.org> Cc: <stable@vger.kernel.org> --- drivers/infiniband/core/user_mad.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)