diff mbox series

net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject

Message ID 20191119095121.6295-1-jouni.hogander@unikie.com
State Changes Requested
Delegated to: David Miller
Headers show
Series net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject | expand

Commit Message

Jouni =?utf-8?Q?H=C3=B6gander?= Nov. 19, 2019, 9:51 a.m. UTC 
From: Jouni Hogander <jouni.hogander@unikie.com>

kobject_init_and_add takes reference even when it fails. This has
to be given up by the caller in error handling. Otherwise memory
allocated by kobject_init_and_add is never freed.

Cc: David Miller <davem@davemloft.net>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
---
 net/core/net-sysfs.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

David Miller Nov. 20, 2019, 1:13 a.m. UTC | #1 
From: jouni.hogander@unikie.com
Date: Tue, 19 Nov 2019 11:51:21 +0200

> kobject_init_and_add takes reference even when it fails.

I see this in the comment above kobject_init_and_add() but not in the
code.

Where does the implementation of kobject_init_and_add() actually take
such a reference?

Did you discover this by code inspection, or by an actual bug that was
triggered?  If by an actual bug, please provide the OOPS and/or checker
trace that indicated a leak was happening here.

I don't see anything actually wrong here because kobject_init_and_add()
doesn't actually seem to do what it's comment suggests...
Jouni =?utf-8?Q?H=C3=B6gander?= Nov. 20, 2019, 6:59 a.m. UTC | #2 
David Miller <davem@davemloft.net> writes:

> From: jouni.hogander@unikie.com
> Date: Tue, 19 Nov 2019 11:51:21 +0200
>
>> kobject_init_and_add takes reference even when it fails.
>
> I see this in the comment above kobject_init_and_add() but not in the
> code.
>
> Where does the implementation of kobject_init_and_add() actually take
> such a reference?

kobject_init_and_add -> kobject_init -> kobject_init_internal -> kref_init

kref_init initializes ref_count as 1.

>
> Did you discover this by code inspection, or by an actual bug that was
> triggered?  If by an actual bug, please provide the OOPS and/or checker
> trace that indicated a leak was happening here.

Originally found it via memory leak identified by Syzkaller. I will
submit new one with memleak dump.

>
> I don't see anything actually wrong here because kobject_init_and_add()
> doesn't actually seem to do what it's comment suggests...

See the code path above.

BR,

Jouni Högander
diff mbox series

Patch

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 865ba6ca16eb..4f404bf33e44 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -923,21 +923,23 @@  static int rx_queue_add_kobject(struct net_device *dev, int index)
 	error = kobject_init_and_add(kobj, &rx_queue_ktype, NULL,
 				     "rx-%u", index);
 	if (error)
-		return error;
+		goto err;
 
 	dev_hold(queue->dev);
 
 	if (dev->sysfs_rx_queue_group) {
 		error = sysfs_create_group(kobj, dev->sysfs_rx_queue_group);
-		if (error) {
-			kobject_put(kobj);
-			return error;
-		}
+		if (error)
+			goto err;
 	}
 
 	kobject_uevent(kobj, KOBJ_ADD);
 
 	return error;
+
+err:
+	kobject_put(kobj);
+	return error;
 }
 #endif /* CONFIG_SYSFS */
 
@@ -1461,21 +1463,21 @@  static int netdev_queue_add_kobject(struct net_device *dev, int index)
 	error = kobject_init_and_add(kobj, &netdev_queue_ktype, NULL,
 				     "tx-%u", index);
 	if (error)
-		return error;
+		goto err;
 
 	dev_hold(queue->dev);
 
 #ifdef CONFIG_BQL
 	error = sysfs_create_group(kobj, &dql_group);
-	if (error) {
-		kobject_put(kobj);
-		return error;
-	}
+	if (error)
+		goto err;
 #endif
 
 	kobject_uevent(kobj, KOBJ_ADD);
 
-	return 0;
+err:
+	kobject_put(kobj);
+	return error;
 }
 #endif /* CONFIG_SYSFS */