Fix a use-after-free in ib_umad

PATCH In review Bart Van Assche 4 participants 20 messages May 12, 2014 – Jun 6, 2014 13 messages not in the archive InfiniBand

lore

Share & tools

lore

Bart Van Assche

author

2014-05-12 08:29 UTC #0 lore raw

cover Fix a use-after-free in ib_umad

This patch series consists of the following three patches:

0001-IB-umad-Remove-container_of-NULL-tests.patch 0002-IB-umad-Fix-error-handling.patch 0003-IB-umad-Fix-a-use-after-free.patch
--
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

Bart Van Assche

author

2014-05-12 08:30 UTC #2 lore raw

PATCH 1/3 IB/umad: Remove container_of() != NULL tests

In reply to a message not in the archive

container_of() never returns NULL. Hence remove the code that tests whether container_of() returns NULL.

Signed-off-by Bart Van Assche <[email protected]>

Cc Alex Chiang <[email protected]>

Cc <[email protected]>

---
 drivers/infiniband/core/user_mad.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/infiniband/core/user_mad.c +2 -8

index f0d588f..e61287c 100644
@@ -783,10 +783,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
 
 	mutex_lock(&port->file_mutex);

@@ -880,10 +877,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {

Signature · 6 lines

1.8.4.5

--
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

Bart Van Assche

author

2014-05-12 08:30 UTC #10 lore raw

PATCH 2/3 IB/umad: Fix error handling

In reply to a message not in the archive

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(-)

drivers/infiniband/core/user_mad.c +23 ~~-14~~

index e61287c..5c67d80 100644
@@ -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)

Signature · 6 lines

1.8.4.5

--
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

Bart Van Assche

author

2014-05-12 08:31 UTC #17 lore raw

PATCH 3/3 IB/umad: Fix a use-after-free

In reply to a message not in the archive

Avoid that closing /dev/infiniband/umad<n> or /dev/infiniband/issm<n> triggers a use-after-free. __fput() in fs/file_table.c invokes f_op->release() before it invokes cdev_put(). Make sure that the ib_umad_device structure is freed by the cdev_put() call instead of f_op->release(). This avoids that changing the port mode from IB into Ethernet and back to IB followed by restarting opensmd triggers the following kernel oops:

general protection fault: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff810cc65c>]  [<ffffffff810cc65c>] module_put+0x2c/0x170
Call Trace:
 [<ffffffff81190f20>] cdev_put+0x20/0x30
 [<ffffffff8118e2ce>] __fput+0x1ae/0x1f0
 [<ffffffff8118e35e>] ____fput+0xe/0x10
 [<ffffffff810723bc>] task_work_run+0xac/0xe0
 [<ffffffff81002a9f>] do_notify_resume+0x9f/0xc0
 [<ffffffff814b8398>] int_signal+0x12/0x17

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051

Signed-off-by Bart Van Assche <[email protected]>

Cc <[email protected]>

---
 drivers/infiniband/core/user_mad.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

drivers/infiniband/core/user_mad.c +24 ~~-13~~

index 5c67d80..7bec4ca 100644
@@ -98,7 +98,7 @@ struct ib_umad_port {
 
 struct ib_umad_device {
 	int                  start_port, end_port;
-	struct kref          ref;
+	struct kobject       kobj;
 	struct ib_umad_port  port[0];
 };

@@ -134,14 +134,18 @@ static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
 static void ib_umad_add_one(struct ib_device *device);
 static void ib_umad_remove_one(struct ib_device *device);
 
-static void ib_umad_release_dev(struct kref *ref)
+static void ib_umad_release_dev(struct kobject *kobj)
 {
 	struct ib_umad_device *dev =
-		container_of(ref, struct ib_umad_device, ref);
+		container_of(kobj, struct ib_umad_device, kobj);
 
 	kfree(dev);
 }
 
+static struct kobj_type ib_umad_dev_ktype = {
+	.release = ib_umad_release_dev,
+};
+
 static int hdr_size(struct ib_umad_file *file)
 {
 	return file->use_pkey_index ? sizeof (struct ib_user_mad_hdr) :

@@ -783,7 +787,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret = -ENXIO;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 	mutex_lock(&port->file_mutex);

@@ -812,7 +816,7 @@ out:
 	mutex_unlock(&port->file_mutex);
 
 	if (ret)
-		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+		kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }

@@ -849,7 +853,7 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
 	mutex_unlock(&file->port->file_mutex);
 
 	kfree(file);
-	kref_put(&dev->ref, ib_umad_release_dev);
+	kobject_put(&dev->kobj);
 
 	return 0;
 }

@@ -877,7 +881,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {

@@ -903,7 +907,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 
 fail:
 	if (ret)
-		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+		kobject_put(&port->umad_dev->kobj);
 	return ret;
 
 clr_sm_cap:

@@ -930,7 +934,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 
 	up(&port->sm_sem);
 
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }

@@ -998,6 +1002,7 @@ static int find_overflow_devnum(void)
 }
 
 static int ib_umad_init_port(struct ib_device *device, int port_num,
+			     struct ib_umad_device *umad_dev,
 			     struct ib_umad_port *port)
 {
 	int devnum;

@@ -1063,6 +1068,11 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 	if (device_create_file(port->sm_dev, &dev_attr_port))
 		goto err_sm_dev;
 
+	port->cdev.kobj.parent = &umad_dev->kobj;
+	kobject_get(&umad_dev->kobj);
+	port->sm_cdev.kobj.parent = &umad_dev->kobj;
+	kobject_get(&umad_dev->kobj);
+
 	return 0;
 
 err_sm_dev:

@@ -1141,7 +1151,7 @@ static void ib_umad_add_one(struct ib_device *device)
 	if (!umad_dev)
 		return;
 
-	kref_init(&umad_dev->ref);
+	kobject_init(&umad_dev->kobj, &ib_umad_dev_ktype);
 
 	umad_dev->start_port = s;
 	umad_dev->end_port   = e;

@@ -1149,7 +1159,8 @@ static void ib_umad_add_one(struct ib_device *device)
 	for (i = s; i <= e; ++i) {
 		umad_dev->port[i - s].umad_dev = umad_dev;
 
-		if (ib_umad_init_port(device, i, &umad_dev->port[i - s]))
+		if (ib_umad_init_port(device, i, umad_dev,
+				      &umad_dev->port[i - s]))
 			goto err;
 	}

@@ -1161,7 +1172,7 @@ err:
 	while (--i >= s)
 		ib_umad_kill_port(&umad_dev->port[i - s]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static void ib_umad_remove_one(struct ib_device *device)

@@ -1175,7 +1186,7 @@ static void ib_umad_remove_one(struct ib_device *device)
 	for (i = 0; i <= umad_dev->end_port - umad_dev->start_port; ++i)
 		ib_umad_kill_port(&umad_dev->port[i]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static char *umad_devnode(struct device *dev, umode_t *mode)

Signature · 6 lines

1.8.4.5

--
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

Bart Van Assche

author

2014-05-12 09:17 UTC #9 lore raw

PATCH 1/3 IB/umad: Remove container_of() != NULL tests

In reply to a message not in the archive

container_of() never returns NULL. Hence remove the code that tests whether container_of() returns NULL.

Signed-off-by Bart Van Assche <[email protected]>

Cc Alex Chiang <[email protected]>

Cc <[email protected]>

---
 drivers/infiniband/core/user_mad.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/infiniband/core/user_mad.c +1 -4

index f0d588f..e61287c 100644
@@ -783,10 +783,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret;
  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
  	mutex_lock(&port->file_mutex);
 @@ -880,10 +877,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;
  	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);

- if (port)
- kref_get(&port->umad_dev->ref);
- else
- return -ENXIO;
+ kref_get(&port->umad_dev->ref);
if (filp->f_flags & O_NONBLOCK) {
if (down_trylock(&port->sm_sem)) {

Signature · 6 lines

1.8.4.5

--
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

Yann Droneaud

2014-05-12 10:04 UTC #4 lore raw

Re: [PATCH 1/3] IB/umad: Remove container_of() != NULL tests

In reply to a message not in the archive

Hi,

Quoted 6 lines · Bart Van Assche

container_of() never returns NULL. Hence remove the code that tests whether container_of() returns NULL.
Signed-off-by Bart Van Assche <[email protected]>
Cc Alex Chiang <[email protected]>
Cc <[email protected]>

The patch seems fine.

But I don't think this one qualify for linux-stable.

Quoted 22 lines

---
 drivers/infiniband/core/user_mad.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/infiniband/core/user_mad.c +1 -4

index f0d588f..e61287c 100644
@@ -783,10 +783,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
 
 	mutex_lock(&port->file_mutex);

@@ -880,10 +877,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;

drivers/infiniband/core/user_mad.c static int ib_umad_sm_open(struct inode *inode, struct file *filp) · line 883

Quoted 10 lines

drivers/infiniband/core/user_mad.c +1 -4

 
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {

Regards.

Signature · 8 lines

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

Yann Droneaud

2014-05-12 10:18 UTC #12 lore raw

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

In reply to a message not in the archive

Hi,

Quoted 16 lines · Bart Van Assche

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(-)
drivers/infiniband/core/user_mad.c +0 -0
index e61287c..5c67d80 100644

drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 784

Quoted 6 lines

drivers/infiniband/core/user_mad.c +1 -1

@@ -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,

drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 793

Quoted 12 lines

drivers/infiniband/core/user_mad.c +2 -3

 	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:

drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 798

Quoted 4 lines

drivers/infiniband/core/user_mad.c +0 -3

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

keep it here.

Quoted 38 lines

drivers/infiniband/core/user_mad.c +11 -7

+	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)

drivers/infiniband/core/user_mad.c static int ib_umad_sm_open(struct inode *inode, struct file *filp) · line 915

Quoted 10 lines

drivers/infiniband/core/user_mad.c +9 -0

+		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.

drivers/infiniband/core/user_mad.c static int ib_umad_sm_open(struct inode *inode, struct file *filp) · line 918

Quoted 3 lines

drivers/infiniband/core/user_mad.c +0 -0

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

Regards.

Signature · 9 lines

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

Bart Van Assche

author

2014-05-12 10:32 UTC #14 lore raw

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

In reply to a message not in the archive

Quoted 12 lines · Yann Droneaud

Quoted 11 lines · Bart Van Assche

drivers/infiniband/core/user_mad.c +1 -3

 	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;
-	}

Quoted 8 lines

drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 793
Quoted 1 line
drivers/infiniband/core/user_mad.c +1 -0
+	ret = -ENOMEM;
especially here: I think it should be moved in the error handling path:
drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 798
Quoted 4 lines
drivers/infiniband/core/user_mad.c +0 -3
 	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bart Van Assche

author

2014-05-12 10:36 UTC #6 lore raw

Re: [PATCH 1/3] IB/umad: Remove container_of() != NULL tests

In reply to a message not in the archive

Quoted 10 lines · Yann Droneaud

Quoted 6 lines · Bart Van Assche
container_of() never returns NULL. Hence remove the code that tests whether container_of() returns NULL.
Signed-off-by Bart Van Assche <[email protected]>
Cc Alex Chiang <[email protected]>
Cc <[email protected]>
The patch seems fine.
But I don't think this one qualify for linux-stable.

One chunk in patch 3/3 touches code that is also touched by this patch. This means that if only patches 2/3 and 3/3 would be sent to stable that these wouldn't apply properly. Hence the "Cc: stable" tag in this patch.

Bart.

--
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

Yann Droneaud

2014-05-12 12:35 UTC #16 lore raw

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

In reply to a message not in the archive

Quoted 22 lines · Bart Van Assche

Quoted 12 lines · Yann Droneaud
Quoted 11 lines · Bart Van Assche
drivers/infiniband/core/user_mad.c +1 -3
 	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;
-	}
Quoted 8 lines
drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 793
Quoted 1 line
drivers/infiniband/core/user_mad.c +1 -0
+	ret = -ENOMEM;
especially here: I think it should be moved in the error handling path:
drivers/infiniband/core/user_mad.c static int ib_umad_open(struct inode *inode, struct file *filp) · line 798
Quoted 4 lines
drivers/infiniband/core/user_mad.c +0 -3
 	file = kzalloc(sizeof *file, GFP_KERNEL);
-	if (!file) {
-		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
-		ret = -ENOMEM;
keep it here.

Quoted 4 lines

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.

Signature · 8 lines

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

Yann Droneaud

2014-05-12 12:40 UTC #8 lore raw

Re: [PATCH 1/3] IB/umad: Remove container_of() != NULL tests

In reply to a message not in the archive

Quoted 8 lines · Bart Van Assche

Quoted 7 lines · Yann Droneaud
Quoted 6 lines · Bart Van Assche
container_of() never returns NULL. Hence remove the code that tests whether container_of() returns NULL.
Signed-off-by Bart Van Assche <[email protected]>
Cc Alex Chiang <[email protected]>
Cc <[email protected]>

Quoted 8 lines

Quoted 3 lines
The patch seems fine.
But I don't think this one qualify for linux-stable.
One chunk in patch 3/3 touches code that is also touched by this patch. This means that if only patches 2/3 and 3/3 would be sent to stable that these wouldn't apply properly. Hence the "Cc: stable" tag in this patch.

You could re-order the patches to make this one apply on top of the two others.

Anyway, it's a minor issue that doesn't justifiy by itself to respin the patchset. But if you're going to take account of my others remarks, it might be worth to reorder the patchset.

Regards.

Signature · 8 lines

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

Bart Van Assche

author

2014-05-16 11:03 UTC #18 lore raw

v3 cover Fix a use-after-free in ib_umad

Changes compared to version 2 of this patch series:
* Converted explicit kobject() get calls into implicit calls by moving the kobj.parent assignments in front of the corresponding cdev_add() calls.

Changes compared to version 1 of this patch series:
* Folded the first patch into the second.
* Implemented Yann's suggestion to drop the test of "ret" in the non-error path of ib_umad_sm_open().
* Simplified the implementation of ib_umad_open() and ib_umad_sm_open() further.

This patch series consists of the following two patches:
0001-IB-umad-Fix-error-handling.patch 0002-IB-umad-Fix-a-use-after-free.patch
--
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

Bart Van Assche

author

2014-05-16 11:04 UTC #20 lore raw

PATCH v3 1/2 IB/umad: Fix error handling

In reply to a message not in the archive

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. Since container_of() never returns NULL, remove the code that tests whether container_of() returns NULL. Note: moving the kref_get() call from the start of ib_umad_*open() to the end is safe since it is the responsibility of the caller of these functions to ensure that the cdev pointer remains valid until at least when these functions return.

Signed-off-by Bart Van Assche <[email protected]>

Cc Alex Chiang <[email protected]>

Cc Yann Droneaud <[email protected]>

Cc <[email protected]>

---
 drivers/infiniband/core/user_mad.c | 58 ++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 25 deletions(-)

drivers/infiniband/core/user_mad.c +33 ~~-25~~

index f0d588f..2b3dfcc 100644
@@ -780,27 +780,19 @@ 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);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
 
 	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);

@@ -815,9 +807,20 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 
 	ret = nonseekable_open(inode, filp);
 
+	if (ret)
+		goto del;
+
+	kref_get(&port->umad_dev->ref);
+
 out:
 	mutex_unlock(&port->file_mutex);
+
 	return ret;
+
+del:
+	list_del(&file->port_list);
+	kfree(file);
+	goto out;
 }
 
 static int ib_umad_close(struct inode *inode, struct file *filp)

@@ -880,36 +883,41 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {
 			ret = -EAGAIN;
-			goto fail;
+			goto out;
 		}
 	} else {
 		if (down_interruptible(&port->sm_sem)) {
 			ret = -ERESTARTSYS;
-			goto fail;
+			goto out;
 		}
 	}
 
 	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);
+	kref_get(&port->umad_dev->ref);
+
+out:
 	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 out;
 }
 
 static int ib_umad_sm_close(struct inode *inode, struct file *filp)

Signature · 6 lines

1.8.4.5

--
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

Bart Van Assche

author

2014-05-16 11:05 UTC #27 lore raw

PATCH v3 2/2 IB/umad: Fix a use-after-free

In reply to a message not in the archive

general protection fault: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff810cc65c>]  [<ffffffff810cc65c>] module_put+0x2c/0x170
Call Trace:
 [<ffffffff81190f20>] cdev_put+0x20/0x30
 [<ffffffff8118e2ce>] __fput+0x1ae/0x1f0
 [<ffffffff8118e35e>] ____fput+0xe/0x10
 [<ffffffff810723bc>] task_work_run+0xac/0xe0
 [<ffffffff81002a9f>] do_notify_resume+0x9f/0xc0
 [<ffffffff814b8398>] int_signal+0x12/0x17

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051

Signed-off-by Bart Van Assche <[email protected]>

Cc Alex Chiang <[email protected]>

Cc Yann Droneaud <[email protected]>

Cc <[email protected]>

---
 drivers/infiniband/core/user_mad.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

drivers/infiniband/core/user_mad.c +19 ~~-11~~

index 2b3dfcc..4ac0d42 100644
@@ -98,7 +98,7 @@ struct ib_umad_port {
 
 struct ib_umad_device {
 	int                  start_port, end_port;
-	struct kref          ref;
+	struct kobject       kobj;
 	struct ib_umad_port  port[0];
 };

@@ -134,14 +134,18 @@ static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
 static void ib_umad_add_one(struct ib_device *device);
 static void ib_umad_remove_one(struct ib_device *device);
 
-static void ib_umad_release_dev(struct kref *ref)
+static void ib_umad_release_dev(struct kobject *kobj)
 {
 	struct ib_umad_device *dev =
-		container_of(ref, struct ib_umad_device, ref);
+		container_of(kobj, struct ib_umad_device, kobj);
 
 	kfree(dev);
 }
 
+static struct kobj_type ib_umad_dev_ktype = {
+	.release = ib_umad_release_dev,
+};
+
 static int hdr_size(struct ib_umad_file *file)
 {
 	return file->use_pkey_index ? sizeof (struct ib_user_mad_hdr) :

@@ -810,7 +814,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto del;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	mutex_unlock(&port->file_mutex);

@@ -855,7 +859,7 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
 	mutex_unlock(&file->port->file_mutex);
 
 	kfree(file);
-	kref_put(&dev->ref, ib_umad_release_dev);
+	kobject_put(&dev->kobj);
 
 	return 0;
 }

@@ -906,7 +910,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto clr_sm_cap;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	return ret;

@@ -935,7 +939,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 
 	up(&port->sm_sem);
 
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }

@@ -1003,6 +1007,7 @@ static int find_overflow_devnum(void)
 }
 
 static int ib_umad_init_port(struct ib_device *device, int port_num,
+			     struct ib_umad_device *umad_dev,
 			     struct ib_umad_port *port)
 {
 	int devnum;

@@ -1035,6 +1040,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 
 	cdev_init(&port->cdev, &umad_fops);
 	port->cdev.owner = THIS_MODULE;
+	port->cdev.kobj.parent = &umad_dev->kobj;
 	kobject_set_name(&port->cdev.kobj, "umad%d", port->dev_num);
 	if (cdev_add(&port->cdev, base, 1))
 		goto err_cdev;

@@ -1053,6 +1059,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 	base += IB_UMAD_MAX_PORTS;
 	cdev_init(&port->sm_cdev, &umad_sm_fops);
 	port->sm_cdev.owner = THIS_MODULE;
+	port->sm_cdev.kobj.parent = &umad_dev->kobj;
 	kobject_set_name(&port->sm_cdev.kobj, "issm%d", port->dev_num);
 	if (cdev_add(&port->sm_cdev, base, 1))
 		goto err_sm_cdev;

@@ -1146,7 +1153,7 @@ static void ib_umad_add_one(struct ib_device *device)
 	if (!umad_dev)
 		return;
 
-	kref_init(&umad_dev->ref);
+	kobject_init(&umad_dev->kobj, &ib_umad_dev_ktype);
 
 	umad_dev->start_port = s;
 	umad_dev->end_port   = e;

@@ -1154,7 +1161,8 @@ static void ib_umad_add_one(struct ib_device *device)
 	for (i = s; i <= e; ++i) {
 		umad_dev->port[i - s].umad_dev = umad_dev;
 
-		if (ib_umad_init_port(device, i, &umad_dev->port[i - s]))
+		if (ib_umad_init_port(device, i, umad_dev,
+				      &umad_dev->port[i - s]))
 			goto err;
 	}

@@ -1166,7 +1174,7 @@ err:
 	while (--i >= s)
 		ib_umad_kill_port(&umad_dev->port[i - s]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static void ib_umad_remove_one(struct ib_device *device)

@@ -1180,7 +1188,7 @@ static void ib_umad_remove_one(struct ib_device *device)
 	for (i = 0; i <= umad_dev->end_port - umad_dev->start_port; ++i)
 		ib_umad_kill_port(&umad_dev->port[i]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static char *umad_devnode(struct device *dev, umode_t *mode)

Signature · 6 lines

1.8.4.5

--
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

Yann Droneaud

Reviewed-by

2014-05-16 12:28 UTC #29 lore raw

Re: [PATCH v3 2/2] IB/umad: Fix a use-after-free

In reply to a message not in the archive

Quoted 150 lines · Bart Van Assche

general protection fault: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff810cc65c>]  [<ffffffff810cc65c>] module_put+0x2c/0x170
Call Trace:
 [<ffffffff81190f20>] cdev_put+0x20/0x30
 [<ffffffff8118e2ce>] __fput+0x1ae/0x1f0
 [<ffffffff8118e35e>] ____fput+0xe/0x10
 [<ffffffff810723bc>] task_work_run+0xac/0xe0
 [<ffffffff81002a9f>] do_notify_resume+0x9f/0xc0
 [<ffffffff814b8398>] int_signal+0x12/0x17

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051

Signed-off-by Bart Van Assche <[email protected]>

Cc Alex Chiang <[email protected]>

Cc Yann Droneaud <[email protected]>

Cc <[email protected]>

---
 drivers/infiniband/core/user_mad.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

drivers/infiniband/core/user_mad.c +18 ~~-10~~

index 2b3dfcc..4ac0d42 100644
@@ -98,7 +98,7 @@ struct ib_umad_port {
 
 struct ib_umad_device {
 	int                  start_port, end_port;
-	struct kref          ref;
+	struct kobject       kobj;
 	struct ib_umad_port  port[0];
 };

@@ -134,14 +134,18 @@ static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
 static void ib_umad_add_one(struct ib_device *device);
 static void ib_umad_remove_one(struct ib_device *device);
 
-static void ib_umad_release_dev(struct kref *ref)
+static void ib_umad_release_dev(struct kobject *kobj)
 {
 	struct ib_umad_device *dev =
-		container_of(ref, struct ib_umad_device, ref);
+		container_of(kobj, struct ib_umad_device, kobj);
 
 	kfree(dev);
 }
 
+static struct kobj_type ib_umad_dev_ktype = {
+	.release = ib_umad_release_dev,
+};
+
 static int hdr_size(struct ib_umad_file *file)
 {
 	return file->use_pkey_index ? sizeof (struct ib_user_mad_hdr) :

@@ -810,7 +814,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto del;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	mutex_unlock(&port->file_mutex);

@@ -855,7 +859,7 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
 	mutex_unlock(&file->port->file_mutex);
 
 	kfree(file);
-	kref_put(&dev->ref, ib_umad_release_dev);
+	kobject_put(&dev->kobj);
 
 	return 0;
 }

@@ -906,7 +910,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto clr_sm_cap;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	return ret;

@@ -935,7 +939,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 
 	up(&port->sm_sem);
 
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }

@@ -1003,6 +1007,7 @@ static int find_overflow_devnum(void)
 }
 
 static int ib_umad_init_port(struct ib_device *device, int port_num,
+			     struct ib_umad_device *umad_dev,
 			     struct ib_umad_port *port)
 {
 	int devnum;

@@ -1035,6 +1040,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 
 	cdev_init(&port->cdev, &umad_fops);
 	port->cdev.owner = THIS_MODULE;
+	port->cdev.kobj.parent = &umad_dev->kobj;
 	kobject_set_name(&port->cdev.kobj, "umad%d", port->dev_num);
 	if (cdev_add(&port->cdev, base, 1))
 		goto err_cdev;

@@ -1053,6 +1059,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 	base += IB_UMAD_MAX_PORTS;
 	cdev_init(&port->sm_cdev, &umad_sm_fops);
 	port->sm_cdev.owner = THIS_MODULE;
+	port->sm_cdev.kobj.parent = &umad_dev->kobj;
 	kobject_set_name(&port->sm_cdev.kobj, "issm%d", port->dev_num);
 	if (cdev_add(&port->sm_cdev, base, 1))
 		goto err_sm_cdev;

@@ -1146,7 +1153,7 @@ static void ib_umad_add_one(struct ib_device *device)
 	if (!umad_dev)
 		return;
 
-	kref_init(&umad_dev->ref);
+	kobject_init(&umad_dev->kobj, &ib_umad_dev_ktype);
 
 	umad_dev->start_port = s;
 	umad_dev->end_port   = e;

@@ -1154,7 +1161,8 @@ static void ib_umad_add_one(struct ib_device *device)
 	for (i = s; i <= e; ++i) {
 		umad_dev->port[i - s].umad_dev = umad_dev;
 
-		if (ib_umad_init_port(device, i, &umad_dev->port[i - s]))
+		if (ib_umad_init_port(device, i, umad_dev,
+				      &umad_dev->port[i - s]))
 			goto err;
 	}

@@ -1166,7 +1174,7 @@ err:
 	while (--i >= s)
 		ib_umad_kill_port(&umad_dev->port[i - s]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static void ib_umad_remove_one(struct ib_device *device)

drivers/infiniband/core/user_mad.c static void ib_umad_remove_one(struct ib_device *device) · line 1194

Quoted 8 lines

drivers/infiniband/core/user_mad.c +1 -1

@@ -1180,7 +1188,7 @@ static void ib_umad_remove_one(struct ib_device *device)
 	for (i = 0; i <= umad_dev->end_port - umad_dev->start_port; ++i)
 		ib_umad_kill_port(&umad_dev->port[i]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static char *umad_devnode(struct device *dev, umode_t *mode)

There's now a equal number of kobject_init() + kobject_get() and kobject_put() (not counting one in the error path), so this sound well balanced.

Regards.

Reviewed-by Yann Droneaud <[email protected]>

Signature · 8 lines

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

Yann Droneaud

2014-05-20 08:33 UTC #22 lore raw

PATCH 1/1 IB/umad: Fix error handling

In reply to a message not in the archive

From: Bart Van Assche <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

Cc Alex Chiang <[email protected]>

Cc Yann Droneaud <[email protected]>

Cc <[email protected]>

[[email protected]: rework a bit to reduce the amount of code changed]

Signed-off-by Yann Droneaud <[email protected]>

---
Hi Bart,

Please find a slightly modified version of your patch to simplify a bit the error paths (no backward goto's) and to reduces the amount of lines touched.

I wasn't able to explain it clearly enough in the previous patch review, so I've made the changes directly in the file and propose the modified patch for you to review.

As it's only suggestion, feel free to integrate the changes in your patch or discard them.

Regards.

 drivers/infiniband/core/user_mad.c | 50 +++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 22 deletions(-)

drivers/infiniband/core/user_mad.c +27 ~~-22~~

index f0d588f8859e..055893b870ee 100644
@@ -780,27 +780,19 @@ 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);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
 
 	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,9 +806,16 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	list_add_tail(&file->port_list, &port->file_list);
 
 	ret = nonseekable_open(inode, filp);
+	if (ret) {
+		list_del(&file->port_list);
+		kfree(file);
+		goto out;
+	}
+
+	kref_get(&port->umad_dev->ref);
 
 out:
 	mutex_unlock(&port->file_mutex);
 	return ret;
 }

@@ -880,10 +880,6 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {

@@ -898,17 +894,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 err_up_sem;
 
 	filp->private_data = port;
 
-	return nonseekable_open(inode, filp);
+	ret = nonseekable_open(inode, filp);
+	if (ret)
+		goto err_clr_sm_cap;
+
+	kref_get(&port->umad_dev->ref);
+
+	return 0;
+
+err_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);
+
+err_up_sem:
+	up(&port->sm_sem);
 
 fail:
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
 	return ret;
 }

Signature · 6 lines

1.9.0

--
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

Bart Van Assche

author

2014-05-20 11:25 UTC #24 lore raw

Re: [PATCH v3.1] IB/umad: Fix error handling

In reply to a message not in the archive

Quoted 10 lines · Yann Droneaud

Please find a slightly modified version of your patch to simplify a bit the error paths (no backward goto's) and to reduces the amount of lines touched.
I wasn't able to explain it clearly enough in the previous patch review, so I've made the changes directly in the file and propose the modified patch for you to review.
As it's only suggestion, feel free to integrate the changes in your patch or discard them.

Hello Yann,

It seems like our opinions about backward goto's are different :-) I thought these are common at the end of error handling code in the Linux kernel. Anyway, what matters to me is that a fix gets upstream, not which fix. But please do not expect me to spend more time testing a patch that has been reworked only because of the coding style aspects mentioned in your e-mail.

Bart.

--
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

Yann Droneaud

2014-05-20 11:39 UTC #26 lore raw

Re: [PATCH v3.1] IB/umad: Fix error handling

In reply to a message not in the archive

Hi,

Quoted 12 lines · Bart Van Assche

Quoted 10 lines · Yann Droneaud
Please find a slightly modified version of your patch to simplify a bit the error paths (no backward goto's) and to reduces the amount of lines touched.
I wasn't able to explain it clearly enough in the previous patch review, so I've made the changes directly in the file and propose the modified patch for you to review.
As it's only suggestion, feel free to integrate the changes in your patch or discard them.

Quoted 9 lines

Hello Yann,
It seems like our opinions about backward goto's are different :-) I thought these are common at the end of error handling code in the Linux kernel. Anyway, what matters to me is that a fix gets upstream, not which fix. But please do not expect me to spend more time testing a patch that has been reworked only because of the coding style aspects mentioned in your e-mail.

Having a shorter diff might help to get the patch applied upstream. YMMV.

Regards.

Signature · 8 lines

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

Yann Droneaud

2014-06-06 16:25 UTC #30 lore raw

PATCH RESEND 1/1 IB/umad: Fix a use-after-free

In reply to Yann Droneaud

From: Bart Van Assche <[email protected]>

general protection fault: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff810cc65c>]  [<ffffffff810cc65c>] module_put+0x2c/0x170
Call Trace:
 [<ffffffff81190f20>] cdev_put+0x20/0x30
 [<ffffffff8118e2ce>] __fput+0x1ae/0x1f0
 [<ffffffff8118e35e>] ____fput+0xe/0x10
 [<ffffffff810723bc>] task_work_run+0xac/0xe0
 [<ffffffff81002a9f>] do_notify_resume+0x9f/0xc0
 [<ffffffff814b8398>] int_signal+0x12/0x17

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051

Signed-off-by Bart Van Assche <[email protected]>

Reviewed-by Yann Droneaud <[email protected]>

Cc Alex Chiang <[email protected]>

Cc <[email protected]> # 3.x: 8ec0a0e6b58: IB/umad: Fix error handling

---
Hi Bart and Roland,

It seems this one was not rolled in your latest katamari[1]. Since the original patch didn't apply on top of commit 8ec0a0e6b58 ('IB/umad: Fix error handling'), I've applied it manually and resend it for review and inclusion.

I hope having correctly set the Cc: [email protected] tag to state the dependency on earlier patch.

[1] this term is actually used by Xorg to team to describe the process of gathering each little tiny piece needed for a release :) By the way, it's also a video game.

Regards.

 drivers/infiniband/core/user_mad.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

drivers/infiniband/core/user_mad.c +19 ~~-11~~

index 9bdf576cd4ab..1acb99100556 100644
@@ -98,7 +98,7 @@ struct ib_umad_port {
 
 struct ib_umad_device {
 	int                  start_port, end_port;
-	struct kref          ref;
+	struct kobject       kobj;
 	struct ib_umad_port  port[0];
 };

@@ -134,14 +134,18 @@ static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
 static void ib_umad_add_one(struct ib_device *device);
 static void ib_umad_remove_one(struct ib_device *device);
 
-static void ib_umad_release_dev(struct kref *ref)
+static void ib_umad_release_dev(struct kobject *kobj)
 {
 	struct ib_umad_device *dev =
-		container_of(ref, struct ib_umad_device, ref);
+		container_of(kobj, struct ib_umad_device, kobj);
 
 	kfree(dev);
 }
 
+static struct kobj_type ib_umad_dev_ktype = {
+	.release = ib_umad_release_dev,
+};
+
 static int hdr_size(struct ib_umad_file *file)
 {
 	return file->use_pkey_index ? sizeof (struct ib_user_mad_hdr) :

@@ -812,7 +816,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 		goto out;
 	}
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	mutex_unlock(&port->file_mutex);

@@ -851,7 +855,7 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
 	mutex_unlock(&file->port->file_mutex);
 
 	kfree(file);
-	kref_put(&dev->ref, ib_umad_release_dev);
+	kobject_put(&dev->kobj);
 
 	return 0;
 }

@@ -902,7 +906,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto err_clr_sm_cap;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 	return 0;

@@ -932,7 +936,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 
 	up(&port->sm_sem);
 
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }

@@ -1000,6 +1004,7 @@ static int find_overflow_devnum(void)
 }
 
 static int ib_umad_init_port(struct ib_device *device, int port_num,
+			     struct ib_umad_device *umad_dev,
 			     struct ib_umad_port *port)
 {
 	int devnum;

@@ -1032,6 +1037,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 
 	cdev_init(&port->cdev, &umad_fops);
 	port->cdev.owner = THIS_MODULE;
+	port->cdev.kobj.parent = &umad_dev->kobj;
 	kobject_set_name(&port->cdev.kobj, "umad%d", port->dev_num);
 	if (cdev_add(&port->cdev, base, 1))
 		goto err_cdev;

@@ -1050,6 +1056,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 	base += IB_UMAD_MAX_PORTS;
 	cdev_init(&port->sm_cdev, &umad_sm_fops);
 	port->sm_cdev.owner = THIS_MODULE;
+	port->sm_cdev.kobj.parent = &umad_dev->kobj;
 	kobject_set_name(&port->sm_cdev.kobj, "issm%d", port->dev_num);
 	if (cdev_add(&port->sm_cdev, base, 1))
 		goto err_sm_cdev;

@@ -1143,7 +1150,7 @@ static void ib_umad_add_one(struct ib_device *device)
 	if (!umad_dev)
 		return;
 
-	kref_init(&umad_dev->ref);
+	kobject_init(&umad_dev->kobj, &ib_umad_dev_ktype);
 
 	umad_dev->start_port = s;
 	umad_dev->end_port   = e;

@@ -1151,7 +1158,8 @@ static void ib_umad_add_one(struct ib_device *device)
 	for (i = s; i <= e; ++i) {
 		umad_dev->port[i - s].umad_dev = umad_dev;
 
-		if (ib_umad_init_port(device, i, &umad_dev->port[i - s]))
+		if (ib_umad_init_port(device, i, umad_dev,
+				      &umad_dev->port[i - s]))
 			goto err;
 	}

@@ -1163,7 +1171,7 @@ err:
 	while (--i >= s)
 		ib_umad_kill_port(&umad_dev->port[i - s]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static void ib_umad_remove_one(struct ib_device *device)

@@ -1177,7 +1185,7 @@ static void ib_umad_remove_one(struct ib_device *device)
 	for (i = 0; i <= umad_dev->end_port - umad_dev->start_port; ++i)
 		ib_umad_kill_port(&umad_dev->port[i]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static char *umad_devnode(struct device *dev, umode_t *mode)

Signature · 1 line

1.9.3

Roland Dreier

2014-06-06 18:39 UTC #32 lore raw

Re: [RESEND PATCH] IB/umad: Fix a use-after-free

In reply to a message not in the archive

Quoted 1 line · Yann Droneaud

It seems this one was not rolled in your latest katamari[1].

Thanks, picked it up this time around.

- R.
--
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