* [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave
@ 2020-11-05 8:41 Jamie Iles
2020-11-05 12:49 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Jamie Iles @ 2020-11-05 8:41 UTC (permalink / raw)
To: netdev
Cc: Jamie Iles, Qiushi Wu, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
struct slave device could result in the following splat:
kobject: 'bonding_slave' (00000000cecdd4fe): kobject_release, parent 0000000074ceb2b2 (delayed 1000)
bond0 (unregistering): (slave bond_slave_1): Releasing backup interface
------------[ cut here ]------------
ODEBUG: free active (active state 0) object type: timer_list hint: workqueue_select_cpu_near kernel/workqueue.c:1549 [inline]
ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x98 kernel/workqueue.c:1600
WARNING: CPU: 1 PID: 842 at lib/debugobjects.c:485 debug_print_object+0x180/0x240 lib/debugobjects.c:485
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 842 Comm: kworker/u4:4 Tainted: G S 5.9.0-rc8+ #96
Hardware name: linux,dummy-virt (DT)
Workqueue: netns cleanup_net
Call trace:
dump_backtrace+0x0/0x4d8 include/linux/bitmap.h:239
show_stack+0x34/0x48 arch/arm64/kernel/traps.c:142
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x174/0x1f8 lib/dump_stack.c:118
panic+0x360/0x7a0 kernel/panic.c:231
__warn+0x244/0x2ec kernel/panic.c:600
report_bug+0x240/0x398 lib/bug.c:198
bug_handler+0x50/0xc0 arch/arm64/kernel/traps.c:974
call_break_hook+0x160/0x1d8 arch/arm64/kernel/debug-monitors.c:322
brk_handler+0x30/0xc0 arch/arm64/kernel/debug-monitors.c:329
do_debug_exception+0x184/0x340 arch/arm64/mm/fault.c:864
el1_dbg+0x48/0xb0 arch/arm64/kernel/entry-common.c:65
el1_sync_handler+0x170/0x1c8 arch/arm64/kernel/entry-common.c:93
el1_sync+0x80/0x100 arch/arm64/kernel/entry.S:594
debug_print_object+0x180/0x240 lib/debugobjects.c:485
__debug_check_no_obj_freed lib/debugobjects.c:967 [inline]
debug_check_no_obj_freed+0x200/0x430 lib/debugobjects.c:998
slab_free_hook mm/slub.c:1536 [inline]
slab_free_freelist_hook+0x190/0x210 mm/slub.c:1577
slab_free mm/slub.c:3138 [inline]
kfree+0x13c/0x460 mm/slub.c:4119
bond_free_slave+0x8c/0xf8 drivers/net/bonding/bond_main.c:1492
__bond_release_one+0xe0c/0xec8 drivers/net/bonding/bond_main.c:2190
bond_slave_netdev_event drivers/net/bonding/bond_main.c:3309 [inline]
bond_netdev_event+0x8f0/0xa70 drivers/net/bonding/bond_main.c:3420
notifier_call_chain+0xf0/0x200 kernel/notifier.c:83
__raw_notifier_call_chain kernel/notifier.c:361 [inline]
raw_notifier_call_chain+0x44/0x58 kernel/notifier.c:368
call_netdevice_notifiers_info+0xbc/0x150 net/core/dev.c:2033
call_netdevice_notifiers_extack net/core/dev.c:2045 [inline]
call_netdevice_notifiers net/core/dev.c:2059 [inline]
rollback_registered_many+0x6a4/0xec0 net/core/dev.c:9347
unregister_netdevice_many.part.0+0x2c/0x1c0 net/core/dev.c:10509
unregister_netdevice_many net/core/dev.c:10508 [inline]
default_device_exit_batch+0x294/0x338 net/core/dev.c:10992
ops_exit_list.isra.0+0xec/0x150 net/core/net_namespace.c:189
cleanup_net+0x44c/0x888 net/core/net_namespace.c:603
process_one_work+0x96c/0x18c0 kernel/workqueue.c:2269
worker_thread+0x3f0/0xc30 kernel/workqueue.c:2415
kthread+0x390/0x498 kernel/kthread.c:292
ret_from_fork+0x10/0x18 arch/arm64/kernel/entry.S:925
This is a potential use-after-free if the sysfs nodes are being accessed
whilst removing the struct slave, so wait for the object destruction to
complete before freeing the struct slave itself.
Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
Cc: Qiushi Wu <wu000273@umn.edu>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---
drivers/net/bonding/bond_sysfs_slave.c | 12 ++++++++++++
include/net/bonding.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
index 9b8346638f69..2fdbcf9692c5 100644
--- a/drivers/net/bonding/bond_sysfs_slave.c
+++ b/drivers/net/bonding/bond_sysfs_slave.c
@@ -136,7 +136,15 @@ static const struct sysfs_ops slave_sysfs_ops = {
.show = slave_show,
};
+static void slave_release(struct kobject *kobj)
+{
+ struct slave *slave = to_slave(kobj);
+
+ complete(&slave->kobj_unregister_done);
+}
+
static struct kobj_type slave_ktype = {
+ .release = slave_release,
#ifdef CONFIG_SYSFS
.sysfs_ops = &slave_sysfs_ops,
#endif
@@ -147,10 +155,12 @@ int bond_sysfs_slave_add(struct slave *slave)
const struct slave_attribute **a;
int err;
+ init_completion(&slave->kobj_unregister_done);
err = kobject_init_and_add(&slave->kobj, &slave_ktype,
&(slave->dev->dev.kobj), "bonding_slave");
if (err) {
kobject_put(&slave->kobj);
+ wait_for_completion(&slave->kobj_unregister_done);
return err;
}
@@ -158,6 +168,7 @@ int bond_sysfs_slave_add(struct slave *slave)
err = sysfs_create_file(&slave->kobj, &((*a)->attr));
if (err) {
kobject_put(&slave->kobj);
+ wait_for_completion(&slave->kobj_unregister_done);
return err;
}
}
@@ -173,4 +184,5 @@ void bond_sysfs_slave_del(struct slave *slave)
sysfs_remove_file(&slave->kobj, &((*a)->attr));
kobject_put(&slave->kobj);
+ wait_for_completion(&slave->kobj_unregister_done);
}
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 7d132cc1e584..78d771d2ffd3 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -25,6 +25,7 @@
#include <linux/etherdevice.h>
#include <linux/reciprocal_div.h>
#include <linux/if_link.h>
+#include <linux/completion.h>
#include <net/bond_3ad.h>
#include <net/bond_alb.h>
@@ -182,6 +183,7 @@ struct slave {
#endif
struct delayed_work notify_work;
struct kobject kobj;
+ struct completion kobj_unregister_done;
struct rtnl_link_stats64 slave_stats;
};
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-05 8:41 [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave Jamie Iles
@ 2020-11-05 12:49 ` Eric Dumazet
2020-11-05 18:11 ` Jamie Iles
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2020-11-05 12:49 UTC (permalink / raw)
To: Jamie Iles, netdev
Cc: Qiushi Wu, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
On 11/5/20 9:41 AM, Jamie Iles wrote:
> syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> struct slave device could result in the following splat:
>
>
> This is a potential use-after-free if the sysfs nodes are being accessed
> whilst removing the struct slave, so wait for the object destruction to
> complete before freeing the struct slave itself.
>
> Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
> Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
> Cc: Qiushi Wu <wu000273@umn.edu>
> Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> Cc: Veaceslav Falico <vfalico@gmail.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> ---
> drivers/net/bonding/bond_sysfs_slave.c | 12 ++++++++++++
> include/net/bonding.h | 2 ++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
> index 9b8346638f69..2fdbcf9692c5 100644
> --- a/drivers/net/bonding/bond_sysfs_slave.c
> +++ b/drivers/net/bonding/bond_sysfs_slave.c
> @@ -136,7 +136,15 @@ static const struct sysfs_ops slave_sysfs_ops = {
> .show = slave_show,
> };
>
> +static void slave_release(struct kobject *kobj)
> +{
> + struct slave *slave = to_slave(kobj);
> +
> + complete(&slave->kobj_unregister_done);
> +}
> +
> static struct kobj_type slave_ktype = {
> + .release = slave_release,
> #ifdef CONFIG_SYSFS
> .sysfs_ops = &slave_sysfs_ops,
> #endif
> @@ -147,10 +155,12 @@ int bond_sysfs_slave_add(struct slave *slave)
> const struct slave_attribute **a;
> int err;
>
> + init_completion(&slave->kobj_unregister_done);
> err = kobject_init_and_add(&slave->kobj, &slave_ktype,
> &(slave->dev->dev.kobj), "bonding_slave");
> if (err) {
> kobject_put(&slave->kobj);
> + wait_for_completion(&slave->kobj_unregister_done);
> return err;
> }
>
> @@ -158,6 +168,7 @@ int bond_sysfs_slave_add(struct slave *slave)
> err = sysfs_create_file(&slave->kobj, &((*a)->attr));
> if (err) {
> kobject_put(&slave->kobj);
> + wait_for_completion(&slave->kobj_unregister_done);
> return err;
> }
> }
> @@ -173,4 +184,5 @@ void bond_sysfs_slave_del(struct slave *slave)
> sysfs_remove_file(&slave->kobj, &((*a)->attr));
>
> kobject_put(&slave->kobj);
> + wait_for_completion(&slave->kobj_unregister_done);
> }
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 7d132cc1e584..78d771d2ffd3 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -25,6 +25,7 @@
> #include <linux/etherdevice.h>
> #include <linux/reciprocal_div.h>
> #include <linux/if_link.h>
> +#include <linux/completion.h>
>
> #include <net/bond_3ad.h>
> #include <net/bond_alb.h>
> @@ -182,6 +183,7 @@ struct slave {
> #endif
> struct delayed_work notify_work;
> struct kobject kobj;
> + struct completion kobj_unregister_done;
> struct rtnl_link_stats64 slave_stats;
> };
This seems weird, are we going to wait for a completion while RTNL is held ?
I am pretty sure this could be exploited by malicious user/syzbot.
The .release() handler could instead perform a refcounted
bond_free_slave() action.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-05 12:49 ` Eric Dumazet
@ 2020-11-05 18:11 ` Jamie Iles
2020-11-06 16:17 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Jamie Iles @ 2020-11-05 18:11 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jamie Iles, netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek
Hi Eric,
On Thu, Nov 05, 2020 at 01:49:03PM +0100, Eric Dumazet wrote:
> On 11/5/20 9:41 AM, Jamie Iles wrote:
> > syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> > struct slave device could result in the following splat:
> >
> >
>
> > This is a potential use-after-free if the sysfs nodes are being accessed
> > whilst removing the struct slave, so wait for the object destruction to
> > complete before freeing the struct slave itself.
> >
> > Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
> > Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
> > Cc: Qiushi Wu <wu000273@umn.edu>
> > Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> > Cc: Veaceslav Falico <vfalico@gmail.com>
> > Cc: Andy Gospodarek <andy@greyhouse.net>
> > Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> > ---
...
> This seems weird, are we going to wait for a completion while RTNL is held ?
> I am pretty sure this could be exploited by malicious user/syzbot.
>
> The .release() handler could instead perform a refcounted
> bond_free_slave() action.
Okay, so were you thinking along the lines of this moving the lifetime
of the slave to the kobject?
Thanks,
Jamie
diff --git i/drivers/net/bonding/bond_main.c w/drivers/net/bonding/bond_main.c
index 84ecbc6fa0ff..ea8ecc6e87c2 100644
--- i/drivers/net/bonding/bond_main.c
+++ w/drivers/net/bonding/bond_main.c
@@ -1481,7 +1481,7 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
return slave;
}
-static void bond_free_slave(struct slave *slave)
+void bond_free_slave(struct slave *slave)
{
struct bonding *bond = bond_get_bond_by_slave(slave);
@@ -1691,6 +1691,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
*/
new_slave->queue_id = 0;
+ res = bond_slave_kobj_init(new_slave);
+ if (res)
+ goto err_free;
+
/* Save slave's original mtu and then set it to match the bond */
new_slave->original_mtu = slave_dev->mtu;
res = dev_set_mtu(slave_dev, bond->dev->mtu);
@@ -1912,7 +1916,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
if (bond_dev->flags & IFF_PROMISC) {
res = dev_set_promiscuity(slave_dev, 1);
if (res)
- goto err_sysfs_del;
+ goto err_upper_unlink;
}
/* set allmulti level to new slave */
@@ -1921,7 +1925,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
if (res) {
if (bond_dev->flags & IFF_PROMISC)
dev_set_promiscuity(slave_dev, -1);
- goto err_sysfs_del;
+ goto err_upper_unlink;
}
}
@@ -1961,9 +1965,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
return 0;
/* Undo stages on error */
-err_sysfs_del:
- bond_sysfs_slave_del(new_slave);
-
err_upper_unlink:
bond_upper_dev_unlink(bond, new_slave);
@@ -2007,7 +2008,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
dev_set_mtu(slave_dev, new_slave->original_mtu);
err_free:
- bond_free_slave(new_slave);
+ bond_slave_kobj_put(new_slave);
err_undo_flags:
/* Enslave of first slave has failed and we need to fix master's mac */
@@ -2066,8 +2067,6 @@ static int __bond_release_one(struct net_device *bond_dev,
bond_set_slave_inactive_flags(slave, BOND_SLAVE_NOTIFY_NOW);
- bond_sysfs_slave_del(slave);
-
/* recompute stats just before removing the slave */
bond_get_stats(bond->dev, &bond->bond_stats);
@@ -2187,7 +2186,7 @@ static int __bond_release_one(struct net_device *bond_dev,
if (!netif_is_bond_master(slave_dev))
slave_dev->priv_flags &= ~IFF_BONDING;
- bond_free_slave(slave);
+ bond_slave_kobj_put(slave);
return 0;
}
diff --git i/drivers/net/bonding/bond_sysfs_slave.c w/drivers/net/bonding/bond_sysfs_slave.c
index 9b8346638f69..67732078ef26 100644
--- i/drivers/net/bonding/bond_sysfs_slave.c
+++ w/drivers/net/bonding/bond_sysfs_slave.c
@@ -136,24 +136,35 @@ static const struct sysfs_ops slave_sysfs_ops = {
.show = slave_show,
};
+static void slave_release(struct kobject *kobj)
+{
+ struct slave *slave = to_slave(kobj);
+
+ bond_free_slave(slave);
+}
+
static struct kobj_type slave_ktype = {
+ .release = slave_release,
#ifdef CONFIG_SYSFS
.sysfs_ops = &slave_sysfs_ops,
#endif
};
+int bond_slave_kobj_init(struct slave *slave)
+{
+ int err = kobject_init_and_add(&slave->kobj, &slave_ktype,
+ &(slave->dev->dev.kobj), "bonding_slave");
+ if (err)
+ kobject_put(&slave->kobj);
+
+ return err;
+}
+
int bond_sysfs_slave_add(struct slave *slave)
{
const struct slave_attribute **a;
int err;
- err = kobject_init_and_add(&slave->kobj, &slave_ktype,
- &(slave->dev->dev.kobj), "bonding_slave");
- if (err) {
- kobject_put(&slave->kobj);
- return err;
- }
-
for (a = slave_attrs; *a; ++a) {
err = sysfs_create_file(&slave->kobj, &((*a)->attr));
if (err) {
@@ -165,7 +176,7 @@ int bond_sysfs_slave_add(struct slave *slave)
return 0;
}
-void bond_sysfs_slave_del(struct slave *slave)
+void bond_slave_kobj_put(struct slave *slave)
{
const struct slave_attribute **a;
diff --git i/include/net/bonding.h w/include/net/bonding.h
index 7d132cc1e584..ccb07e3e495e 100644
--- i/include/net/bonding.h
+++ w/include/net/bonding.h
@@ -622,10 +622,12 @@ int bond_create(struct net *net, const char *name);
int bond_create_sysfs(struct bond_net *net);
void bond_destroy_sysfs(struct bond_net *net);
void bond_prepare_sysfs_group(struct bonding *bond);
+int bond_slave_kobj_init(struct slave *slave);
int bond_sysfs_slave_add(struct slave *slave);
-void bond_sysfs_slave_del(struct slave *slave);
+void bond_slave_kobj_put(struct slave *slave);
int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
struct netlink_ext_ack *extack);
+void bond_free_slave(struct slave *slave);
int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb);
int bond_set_carrier(struct bonding *bond);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-05 18:11 ` Jamie Iles
@ 2020-11-06 16:17 ` Eric Dumazet
2020-11-13 17:12 ` [PATCHv2] " Jamie Iles
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2020-11-06 16:17 UTC (permalink / raw)
To: Jamie Iles, Eric Dumazet
Cc: netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
On 11/5/20 7:11 PM, Jamie Iles wrote:
> Hi Eric,
>
> On Thu, Nov 05, 2020 at 01:49:03PM +0100, Eric Dumazet wrote:
>> On 11/5/20 9:41 AM, Jamie Iles wrote:
>>> syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
>>> struct slave device could result in the following splat:
>>>
>>>
>>
>>> This is a potential use-after-free if the sysfs nodes are being accessed
>>> whilst removing the struct slave, so wait for the object destruction to
>>> complete before freeing the struct slave itself.
>>>
>>> Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
>>> Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
>>> Cc: Qiushi Wu <wu000273@umn.edu>
>>> Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>>> Cc: Veaceslav Falico <vfalico@gmail.com>
>>> Cc: Andy Gospodarek <andy@greyhouse.net>
>>> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
>>> ---
> ...
>> This seems weird, are we going to wait for a completion while RTNL is held ?
>> I am pretty sure this could be exploited by malicious user/syzbot.
>>
>> The .release() handler could instead perform a refcounted
>> bond_free_slave() action.
>
> Okay, so were you thinking along the lines of this moving the lifetime
> of the slave to the kobject?
>
This seems a bit too complex for a bug fix.
Instead of adding a completion, you could add a refcount_t, and
make bond_free_slave() a wrapper like
static inline void bond_free_slave(struct slave *slave)
{
if (refcount_dec_and_test(&slave->refcnt))
__bond_free_slave(slave);
}
Once the kobj is successfully activated, you would
need a refcount_inc(&slave->refcount);
Total patch would be smaller and easier to review.
The kobj .release handler would simply call bond_free_slave(slave);
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-06 16:17 ` Eric Dumazet
@ 2020-11-13 17:12 ` Jamie Iles
2020-11-17 20:34 ` Jakub Kicinski
2020-11-20 14:28 ` [PATCHv3] " Jamie Iles
0 siblings, 2 replies; 10+ messages in thread
From: Jamie Iles @ 2020-11-13 17:12 UTC (permalink / raw)
To: netdev
Cc: Jamie Iles, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, Eric Dumazet
syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
struct slave device could result in the following splat:
kobject: 'bonding_slave' (00000000cecdd4fe): kobject_release, parent 0000000074ceb2b2 (delayed 1000)
bond0 (unregistering): (slave bond_slave_1): Releasing backup interface
------------[ cut here ]------------
ODEBUG: free active (active state 0) object type: timer_list hint: workqueue_select_cpu_near kernel/workqueue.c:1549 [inline]
ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x98 kernel/workqueue.c:1600
WARNING: CPU: 1 PID: 842 at lib/debugobjects.c:485 debug_print_object+0x180/0x240 lib/debugobjects.c:485
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 842 Comm: kworker/u4:4 Tainted: G S 5.9.0-rc8+ #96
Hardware name: linux,dummy-virt (DT)
Workqueue: netns cleanup_net
Call trace:
dump_backtrace+0x0/0x4d8 include/linux/bitmap.h:239
show_stack+0x34/0x48 arch/arm64/kernel/traps.c:142
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x174/0x1f8 lib/dump_stack.c:118
panic+0x360/0x7a0 kernel/panic.c:231
__warn+0x244/0x2ec kernel/panic.c:600
report_bug+0x240/0x398 lib/bug.c:198
bug_handler+0x50/0xc0 arch/arm64/kernel/traps.c:974
call_break_hook+0x160/0x1d8 arch/arm64/kernel/debug-monitors.c:322
brk_handler+0x30/0xc0 arch/arm64/kernel/debug-monitors.c:329
do_debug_exception+0x184/0x340 arch/arm64/mm/fault.c:864
el1_dbg+0x48/0xb0 arch/arm64/kernel/entry-common.c:65
el1_sync_handler+0x170/0x1c8 arch/arm64/kernel/entry-common.c:93
el1_sync+0x80/0x100 arch/arm64/kernel/entry.S:594
debug_print_object+0x180/0x240 lib/debugobjects.c:485
__debug_check_no_obj_freed lib/debugobjects.c:967 [inline]
debug_check_no_obj_freed+0x200/0x430 lib/debugobjects.c:998
slab_free_hook mm/slub.c:1536 [inline]
slab_free_freelist_hook+0x190/0x210 mm/slub.c:1577
slab_free mm/slub.c:3138 [inline]
kfree+0x13c/0x460 mm/slub.c:4119
bond_free_slave+0x8c/0xf8 drivers/net/bonding/bond_main.c:1492
__bond_release_one+0xe0c/0xec8 drivers/net/bonding/bond_main.c:2190
bond_slave_netdev_event drivers/net/bonding/bond_main.c:3309 [inline]
bond_netdev_event+0x8f0/0xa70 drivers/net/bonding/bond_main.c:3420
notifier_call_chain+0xf0/0x200 kernel/notifier.c:83
__raw_notifier_call_chain kernel/notifier.c:361 [inline]
raw_notifier_call_chain+0x44/0x58 kernel/notifier.c:368
call_netdevice_notifiers_info+0xbc/0x150 net/core/dev.c:2033
call_netdevice_notifiers_extack net/core/dev.c:2045 [inline]
call_netdevice_notifiers net/core/dev.c:2059 [inline]
rollback_registered_many+0x6a4/0xec0 net/core/dev.c:9347
unregister_netdevice_many.part.0+0x2c/0x1c0 net/core/dev.c:10509
unregister_netdevice_many net/core/dev.c:10508 [inline]
default_device_exit_batch+0x294/0x338 net/core/dev.c:10992
ops_exit_list.isra.0+0xec/0x150 net/core/net_namespace.c:189
cleanup_net+0x44c/0x888 net/core/net_namespace.c:603
process_one_work+0x96c/0x18c0 kernel/workqueue.c:2269
worker_thread+0x3f0/0xc30 kernel/workqueue.c:2415
kthread+0x390/0x498 kernel/kthread.c:292
ret_from_fork+0x10/0x18 arch/arm64/kernel/entry.S:925
This is a potential use-after-free if the sysfs nodes are being accessed
whilst removing the struct slave, so wait for the object destruction to
complete before freeing the struct slave itself.
Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
Cc: Qiushi Wu <wu000273@umn.edu>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---
v2:
- use a kref for managing the slave lifecycle rather than waiting on a
completion and blocking
drivers/net/bonding/bond_main.c | 19 ++++++++++++++---
drivers/net/bonding/bond_sysfs_slave.c | 28 ++++++++++++++++++--------
include/net/bonding.h | 4 ++++
3 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 84ecbc6fa0ff..66e56642e6c2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1478,11 +1478,14 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
}
INIT_DELAYED_WORK(&slave->notify_work, bond_netdev_notify_work);
+ kref_init(&slave->ref);
+
return slave;
}
-static void bond_free_slave(struct slave *slave)
+static void __bond_free_slave(struct kref *ref)
{
+ struct slave *slave = container_of(ref, struct slave, ref);
struct bonding *bond = bond_get_bond_by_slave(slave);
cancel_delayed_work_sync(&slave->notify_work);
@@ -1492,6 +1495,16 @@ static void bond_free_slave(struct slave *slave)
kfree(slave);
}
+void bond_slave_put_ref(struct slave *slave)
+{
+ kref_put(&slave->ref, __bond_free_slave);
+}
+
+void bond_slave_get_ref(struct slave *slave)
+{
+ kref_get(&slave->ref);
+}
+
static void bond_fill_ifbond(struct bonding *bond, struct ifbond *info)
{
info->bond_mode = BOND_MODE(bond);
@@ -2007,7 +2020,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
dev_set_mtu(slave_dev, new_slave->original_mtu);
err_free:
- bond_free_slave(new_slave);
+ bond_slave_put_ref(new_slave);
err_undo_flags:
/* Enslave of first slave has failed and we need to fix master's mac */
@@ -2187,7 +2200,7 @@ static int __bond_release_one(struct net_device *bond_dev,
if (!netif_is_bond_master(slave_dev))
slave_dev->priv_flags &= ~IFF_BONDING;
- bond_free_slave(slave);
+ bond_slave_put_ref(slave);
return 0;
}
diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
index 9b8346638f69..5f8aac715ee8 100644
--- a/drivers/net/bonding/bond_sysfs_slave.c
+++ b/drivers/net/bonding/bond_sysfs_slave.c
@@ -136,7 +136,15 @@ static const struct sysfs_ops slave_sysfs_ops = {
.show = slave_show,
};
+static void slave_release(struct kobject *kobj)
+{
+ struct slave *slave = to_slave(kobj);
+
+ bond_slave_put_ref(slave);
+}
+
static struct kobj_type slave_ktype = {
+ .release = slave_release,
#ifdef CONFIG_SYSFS
.sysfs_ops = &slave_sysfs_ops,
#endif
@@ -147,22 +155,26 @@ int bond_sysfs_slave_add(struct slave *slave)
const struct slave_attribute **a;
int err;
+ bond_slave_get_ref(slave);
+
err = kobject_init_and_add(&slave->kobj, &slave_ktype,
&(slave->dev->dev.kobj), "bonding_slave");
- if (err) {
- kobject_put(&slave->kobj);
- return err;
- }
+ if (err)
+ goto out_put_slave;
for (a = slave_attrs; *a; ++a) {
err = sysfs_create_file(&slave->kobj, &((*a)->attr));
- if (err) {
- kobject_put(&slave->kobj);
- return err;
- }
+ if (err)
+ goto out_put_slave;
}
return 0;
+
+out_put_slave:
+ kobject_put(&slave->kobj);
+ bond_slave_put_ref(slave);
+
+ return err;
}
void bond_sysfs_slave_del(struct slave *slave)
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 7d132cc1e584..e286ff4e0882 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -25,6 +25,7 @@
#include <linux/etherdevice.h>
#include <linux/reciprocal_div.h>
#include <linux/if_link.h>
+#include <linux/kref.h>
#include <net/bond_3ad.h>
#include <net/bond_alb.h>
@@ -157,6 +158,7 @@ struct bond_parm_tbl {
struct slave {
struct net_device *dev; /* first - useful for panic debug */
struct bonding *bond; /* our master */
+ struct kref ref;
int delay;
/* all three in jiffies */
unsigned long last_link_up;
@@ -649,6 +651,8 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay);
void bond_work_init_all(struct bonding *bond);
+void bond_slave_get_ref(struct slave *slave);
+void bond_slave_put_ref(struct slave *slave);
#ifdef CONFIG_PROC_FS
void bond_create_proc_entry(struct bonding *bond);
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv2] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-13 17:12 ` [PATCHv2] " Jamie Iles
@ 2020-11-17 20:34 ` Jakub Kicinski
2020-11-18 7:58 ` Greg Kroah-Hartman
2020-11-20 14:28 ` [PATCHv3] " Jamie Iles
1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-11-17 20:34 UTC (permalink / raw)
To: Jamie Iles
Cc: netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, Eric Dumazet, Greg Kroah-Hartman
On Fri, 13 Nov 2020 17:12:44 +0000 Jamie Iles wrote:
> syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> struct slave device could result in the following splat:
> This is a potential use-after-free if the sysfs nodes are being accessed
> whilst removing the struct slave, so wait for the object destruction to
> complete before freeing the struct slave itself.
>
> Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
> Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
This code looks surprising, although admittedly my kobj understanding
is cursory at best. So CCing Greg to keep me honest.
kobj itself is a refcounting mechanism. Adding another refcount and
then releasing the reference from .release method of kobject looks like
a pointless duplication.
Just free the object from the .release method. Why not?
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 84ecbc6fa0ff..66e56642e6c2 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1478,11 +1478,14 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
> }
> INIT_DELAYED_WORK(&slave->notify_work, bond_netdev_notify_work);
>
> + kref_init(&slave->ref);
> +
> return slave;
> }
>
> -static void bond_free_slave(struct slave *slave)
> +static void __bond_free_slave(struct kref *ref)
> {
> + struct slave *slave = container_of(ref, struct slave, ref);
> struct bonding *bond = bond_get_bond_by_slave(slave);
>
> cancel_delayed_work_sync(&slave->notify_work);
> @@ -1492,6 +1495,16 @@ static void bond_free_slave(struct slave *slave)
> kfree(slave);
> }
>
> +void bond_slave_put_ref(struct slave *slave)
> +{
> + kref_put(&slave->ref, __bond_free_slave);
> +}
> +
> +void bond_slave_get_ref(struct slave *slave)
> +{
> + kref_get(&slave->ref);
> +}
> +
> static void bond_fill_ifbond(struct bonding *bond, struct ifbond *info)
> {
> info->bond_mode = BOND_MODE(bond);
> @@ -2007,7 +2020,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> dev_set_mtu(slave_dev, new_slave->original_mtu);
>
> err_free:
> - bond_free_slave(new_slave);
> + bond_slave_put_ref(new_slave);
>
> err_undo_flags:
> /* Enslave of first slave has failed and we need to fix master's mac */
> @@ -2187,7 +2200,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> if (!netif_is_bond_master(slave_dev))
> slave_dev->priv_flags &= ~IFF_BONDING;
>
> - bond_free_slave(slave);
> + bond_slave_put_ref(slave);
>
> return 0;
> }
> diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
> index 9b8346638f69..5f8aac715ee8 100644
> --- a/drivers/net/bonding/bond_sysfs_slave.c
> +++ b/drivers/net/bonding/bond_sysfs_slave.c
> @@ -136,7 +136,15 @@ static const struct sysfs_ops slave_sysfs_ops = {
> .show = slave_show,
> };
>
> +static void slave_release(struct kobject *kobj)
> +{
> + struct slave *slave = to_slave(kobj);
> +
> + bond_slave_put_ref(slave);
> +}
> +
> static struct kobj_type slave_ktype = {
> + .release = slave_release,
> #ifdef CONFIG_SYSFS
> .sysfs_ops = &slave_sysfs_ops,
> #endif
> @@ -147,22 +155,26 @@ int bond_sysfs_slave_add(struct slave *slave)
> const struct slave_attribute **a;
> int err;
>
> + bond_slave_get_ref(slave);
> +
> err = kobject_init_and_add(&slave->kobj, &slave_ktype,
> &(slave->dev->dev.kobj), "bonding_slave");
> - if (err) {
> - kobject_put(&slave->kobj);
> - return err;
> - }
> + if (err)
> + goto out_put_slave;
>
> for (a = slave_attrs; *a; ++a) {
> err = sysfs_create_file(&slave->kobj, &((*a)->attr));
> - if (err) {
> - kobject_put(&slave->kobj);
> - return err;
> - }
> + if (err)
> + goto out_put_slave;
> }
>
> return 0;
> +
> +out_put_slave:
> + kobject_put(&slave->kobj);
> + bond_slave_put_ref(slave);
> +
> + return err;
> }
>
> void bond_sysfs_slave_del(struct slave *slave)
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 7d132cc1e584..e286ff4e0882 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -25,6 +25,7 @@
> #include <linux/etherdevice.h>
> #include <linux/reciprocal_div.h>
> #include <linux/if_link.h>
> +#include <linux/kref.h>
>
> #include <net/bond_3ad.h>
> #include <net/bond_alb.h>
> @@ -157,6 +158,7 @@ struct bond_parm_tbl {
> struct slave {
> struct net_device *dev; /* first - useful for panic debug */
> struct bonding *bond; /* our master */
> + struct kref ref;
> int delay;
> /* all three in jiffies */
> unsigned long last_link_up;
> @@ -649,6 +651,8 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
> void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay);
> void bond_work_init_all(struct bonding *bond);
> +void bond_slave_get_ref(struct slave *slave);
> +void bond_slave_put_ref(struct slave *slave);
>
> #ifdef CONFIG_PROC_FS
> void bond_create_proc_entry(struct bonding *bond);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-17 20:34 ` Jakub Kicinski
@ 2020-11-18 7:58 ` Greg Kroah-Hartman
0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-18 7:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jamie Iles, netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, Eric Dumazet
On Tue, Nov 17, 2020 at 12:34:01PM -0800, Jakub Kicinski wrote:
> On Fri, 13 Nov 2020 17:12:44 +0000 Jamie Iles wrote:
> > syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> > struct slave device could result in the following splat:
>
> > This is a potential use-after-free if the sysfs nodes are being accessed
> > whilst removing the struct slave, so wait for the object destruction to
> > complete before freeing the struct slave itself.
> >
> > Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
> > Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
>
> This code looks surprising, although admittedly my kobj understanding
> is cursory at best. So CCing Greg to keep me honest.
>
> kobj itself is a refcounting mechanism. Adding another refcount and
> then releasing the reference from .release method of kobject looks like
> a pointless duplication.
Yes, that's wrong.
> Just free the object from the .release method. Why not?
Correct, that is what should be happening. Otherwise it seems that
something else has a pointer to this object that forgot to increment it?
>
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 84ecbc6fa0ff..66e56642e6c2 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1478,11 +1478,14 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
> > }
> > INIT_DELAYED_WORK(&slave->notify_work, bond_netdev_notify_work);
> >
> > + kref_init(&slave->ref);
> > +
> > return slave;
> > }
> >
> > -static void bond_free_slave(struct slave *slave)
> > +static void __bond_free_slave(struct kref *ref)
> > {
> > + struct slave *slave = container_of(ref, struct slave, ref);
> > struct bonding *bond = bond_get_bond_by_slave(slave);
> >
> > cancel_delayed_work_sync(&slave->notify_work);
> > @@ -1492,6 +1495,16 @@ static void bond_free_slave(struct slave *slave)
> > kfree(slave);
> > }
> >
> > +void bond_slave_put_ref(struct slave *slave)
> > +{
> > + kref_put(&slave->ref, __bond_free_slave);
> > +}
> > +
> > +void bond_slave_get_ref(struct slave *slave)
> > +{
> > + kref_get(&slave->ref);
> > +}
> > +
> > static void bond_fill_ifbond(struct bonding *bond, struct ifbond *info)
> > {
> > info->bond_mode = BOND_MODE(bond);
> > @@ -2007,7 +2020,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > dev_set_mtu(slave_dev, new_slave->original_mtu);
> >
> > err_free:
> > - bond_free_slave(new_slave);
> > + bond_slave_put_ref(new_slave);
> >
> > err_undo_flags:
> > /* Enslave of first slave has failed and we need to fix master's mac */
> > @@ -2187,7 +2200,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> > if (!netif_is_bond_master(slave_dev))
> > slave_dev->priv_flags &= ~IFF_BONDING;
> >
> > - bond_free_slave(slave);
> > + bond_slave_put_ref(slave);
> >
> > return 0;
> > }
> > diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
> > index 9b8346638f69..5f8aac715ee8 100644
> > --- a/drivers/net/bonding/bond_sysfs_slave.c
> > +++ b/drivers/net/bonding/bond_sysfs_slave.c
> > @@ -136,7 +136,15 @@ static const struct sysfs_ops slave_sysfs_ops = {
> > .show = slave_show,
> > };
> >
> > +static void slave_release(struct kobject *kobj)
> > +{
> > + struct slave *slave = to_slave(kobj);
> > +
> > + bond_slave_put_ref(slave);
> > +}
> > +
> > static struct kobj_type slave_ktype = {
> > + .release = slave_release,
> > #ifdef CONFIG_SYSFS
> > .sysfs_ops = &slave_sysfs_ops,
> > #endif
> > @@ -147,22 +155,26 @@ int bond_sysfs_slave_add(struct slave *slave)
> > const struct slave_attribute **a;
> > int err;
> >
> > + bond_slave_get_ref(slave);
> > +
> > err = kobject_init_and_add(&slave->kobj, &slave_ktype,
> > &(slave->dev->dev.kobj), "bonding_slave");
> > - if (err) {
> > - kobject_put(&slave->kobj);
> > - return err;
> > - }
> > + if (err)
> > + goto out_put_slave;
> >
> > for (a = slave_attrs; *a; ++a) {
> > err = sysfs_create_file(&slave->kobj, &((*a)->attr));
> > - if (err) {
> > - kobject_put(&slave->kobj);
> > - return err;
> > - }
> > + if (err)
> > + goto out_put_slave;
> > }
> >
> > return 0;
> > +
> > +out_put_slave:
> > + kobject_put(&slave->kobj);
> > + bond_slave_put_ref(slave);
> > +
> > + return err;
> > }
> >
> > void bond_sysfs_slave_del(struct slave *slave)
> > diff --git a/include/net/bonding.h b/include/net/bonding.h
> > index 7d132cc1e584..e286ff4e0882 100644
> > --- a/include/net/bonding.h
> > +++ b/include/net/bonding.h
> > @@ -25,6 +25,7 @@
> > #include <linux/etherdevice.h>
> > #include <linux/reciprocal_div.h>
> > #include <linux/if_link.h>
> > +#include <linux/kref.h>
> >
> > #include <net/bond_3ad.h>
> > #include <net/bond_alb.h>
> > @@ -157,6 +158,7 @@ struct bond_parm_tbl {
> > struct slave {
> > struct net_device *dev; /* first - useful for panic debug */
> > struct bonding *bond; /* our master */
> > + struct kref ref;
Now you have 2 different reference counts for the same structure, a
guaranteed way to cause total confusion and mistakes about lifetime
rules.
This is not the correct way to fix it, use the reference count in the
kobject, that is what it is there for.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv3] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-13 17:12 ` [PATCHv2] " Jamie Iles
2020-11-17 20:34 ` Jakub Kicinski
@ 2020-11-20 14:28 ` Jamie Iles
2020-11-20 15:39 ` Greg Kroah-Hartman
1 sibling, 1 reply; 10+ messages in thread
From: Jamie Iles @ 2020-11-20 14:28 UTC (permalink / raw)
To: netdev
Cc: Jamie Iles, Greg Kroah-Hartman, Qiushi Wu, Jay Vosburgh,
Veaceslav Falico, Andy Gospodarek
syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
struct slave device could result in the following splat:
kobject: 'bonding_slave' (00000000cecdd4fe): kobject_release, parent 0000000074ceb2b2 (delayed 1000)
bond0 (unregistering): (slave bond_slave_1): Releasing backup interface
------------[ cut here ]------------
ODEBUG: free active (active state 0) object type: timer_list hint: workqueue_select_cpu_near kernel/workqueue.c:1549 [inline]
ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x98 kernel/workqueue.c:1600
WARNING: CPU: 1 PID: 842 at lib/debugobjects.c:485 debug_print_object+0x180/0x240 lib/debugobjects.c:485
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 842 Comm: kworker/u4:4 Tainted: G S 5.9.0-rc8+ #96
Hardware name: linux,dummy-virt (DT)
Workqueue: netns cleanup_net
Call trace:
dump_backtrace+0x0/0x4d8 include/linux/bitmap.h:239
show_stack+0x34/0x48 arch/arm64/kernel/traps.c:142
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x174/0x1f8 lib/dump_stack.c:118
panic+0x360/0x7a0 kernel/panic.c:231
__warn+0x244/0x2ec kernel/panic.c:600
report_bug+0x240/0x398 lib/bug.c:198
bug_handler+0x50/0xc0 arch/arm64/kernel/traps.c:974
call_break_hook+0x160/0x1d8 arch/arm64/kernel/debug-monitors.c:322
brk_handler+0x30/0xc0 arch/arm64/kernel/debug-monitors.c:329
do_debug_exception+0x184/0x340 arch/arm64/mm/fault.c:864
el1_dbg+0x48/0xb0 arch/arm64/kernel/entry-common.c:65
el1_sync_handler+0x170/0x1c8 arch/arm64/kernel/entry-common.c:93
el1_sync+0x80/0x100 arch/arm64/kernel/entry.S:594
debug_print_object+0x180/0x240 lib/debugobjects.c:485
__debug_check_no_obj_freed lib/debugobjects.c:967 [inline]
debug_check_no_obj_freed+0x200/0x430 lib/debugobjects.c:998
slab_free_hook mm/slub.c:1536 [inline]
slab_free_freelist_hook+0x190/0x210 mm/slub.c:1577
slab_free mm/slub.c:3138 [inline]
kfree+0x13c/0x460 mm/slub.c:4119
bond_free_slave+0x8c/0xf8 drivers/net/bonding/bond_main.c:1492
__bond_release_one+0xe0c/0xec8 drivers/net/bonding/bond_main.c:2190
bond_slave_netdev_event drivers/net/bonding/bond_main.c:3309 [inline]
bond_netdev_event+0x8f0/0xa70 drivers/net/bonding/bond_main.c:3420
notifier_call_chain+0xf0/0x200 kernel/notifier.c:83
__raw_notifier_call_chain kernel/notifier.c:361 [inline]
raw_notifier_call_chain+0x44/0x58 kernel/notifier.c:368
call_netdevice_notifiers_info+0xbc/0x150 net/core/dev.c:2033
call_netdevice_notifiers_extack net/core/dev.c:2045 [inline]
call_netdevice_notifiers net/core/dev.c:2059 [inline]
rollback_registered_many+0x6a4/0xec0 net/core/dev.c:9347
unregister_netdevice_many.part.0+0x2c/0x1c0 net/core/dev.c:10509
unregister_netdevice_many net/core/dev.c:10508 [inline]
default_device_exit_batch+0x294/0x338 net/core/dev.c:10992
ops_exit_list.isra.0+0xec/0x150 net/core/net_namespace.c:189
cleanup_net+0x44c/0x888 net/core/net_namespace.c:603
process_one_work+0x96c/0x18c0 kernel/workqueue.c:2269
worker_thread+0x3f0/0xc30 kernel/workqueue.c:2415
kthread+0x390/0x498 kernel/kthread.c:292
ret_from_fork+0x10/0x18 arch/arm64/kernel/entry.S:925
This is a potential use-after-free if the sysfs nodes are being accessed
whilst removing the struct slave, so wait for the object destruction to
complete before freeing the struct slave itself.
Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Qiushi Wu <wu000273@umn.edu>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---
v3:
- have a single object lifecycle in the struct slave and remove the
explicit deallocation and defer that to the kobject
v2:
- use a kref for managing the slave lifecycle rather than waiting on a
completion and blocking
drivers/net/bonding/bond_main.c | 59 ++++++++++++++++++--------
drivers/net/bonding/bond_sysfs_slave.c | 18 +-------
include/net/bonding.h | 8 ++++
3 files changed, 50 insertions(+), 35 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 84ecbc6fa0ff..3c8df8eb8581 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1460,7 +1460,37 @@ static void bond_upper_dev_unlink(struct bonding *bond, struct slave *slave)
slave->dev->flags &= ~IFF_SLAVE;
}
-static struct slave *bond_alloc_slave(struct bonding *bond)
+static void slave_kobj_release(struct kobject *kobj)
+{
+ struct slave *slave = to_slave(kobj);
+ struct bonding *bond = bond_get_bond_by_slave(slave);
+
+ cancel_delayed_work_sync(&slave->notify_work);
+ if (BOND_MODE(bond) == BOND_MODE_8023AD)
+ kfree(SLAVE_AD_INFO(slave));
+
+ kfree(slave);
+}
+
+static struct kobj_type slave_ktype = {
+ .release = slave_kobj_release,
+#ifdef CONFIG_SYSFS
+ .sysfs_ops = &slave_sysfs_ops,
+#endif
+};
+
+static int bond_kobj_init(struct slave *slave)
+{
+ int err = kobject_init_and_add(&slave->kobj, &slave_ktype,
+ &(slave->dev->dev.kobj), "bonding_slave");
+ if (err)
+ kobject_put(&slave->kobj);
+
+ return err;
+}
+
+static struct slave *bond_alloc_slave(struct bonding *bond,
+ struct net_device *slave_dev)
{
struct slave *slave = NULL;
@@ -1468,11 +1498,17 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
if (!slave)
return NULL;
+ slave->bond = bond;
+ slave->dev = slave_dev;
+
+ if (bond_kobj_init(slave))
+ return NULL;
+
if (BOND_MODE(bond) == BOND_MODE_8023AD) {
SLAVE_AD_INFO(slave) = kzalloc(sizeof(struct ad_slave_info),
GFP_KERNEL);
if (!SLAVE_AD_INFO(slave)) {
- kfree(slave);
+ kobject_put(&slave->kobj);
return NULL;
}
}
@@ -1481,17 +1517,6 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
return slave;
}
-static void bond_free_slave(struct slave *slave)
-{
- struct bonding *bond = bond_get_bond_by_slave(slave);
-
- cancel_delayed_work_sync(&slave->notify_work);
- if (BOND_MODE(bond) == BOND_MODE_8023AD)
- kfree(SLAVE_AD_INFO(slave));
-
- kfree(slave);
-}
-
static void bond_fill_ifbond(struct bonding *bond, struct ifbond *info)
{
info->bond_mode = BOND_MODE(bond);
@@ -1678,14 +1703,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
goto err_undo_flags;
}
- new_slave = bond_alloc_slave(bond);
+ new_slave = bond_alloc_slave(bond, slave_dev);
if (!new_slave) {
res = -ENOMEM;
goto err_undo_flags;
}
- new_slave->bond = bond;
- new_slave->dev = slave_dev;
/* Set the new_slave's queue_id to be zero. Queue ID mapping
* is set via sysfs or module option if desired.
*/
@@ -2007,7 +2030,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
dev_set_mtu(slave_dev, new_slave->original_mtu);
err_free:
- bond_free_slave(new_slave);
+ kobject_put(&new_slave->kobj);
err_undo_flags:
/* Enslave of first slave has failed and we need to fix master's mac */
@@ -2187,7 +2210,7 @@ static int __bond_release_one(struct net_device *bond_dev,
if (!netif_is_bond_master(slave_dev))
slave_dev->priv_flags &= ~IFF_BONDING;
- bond_free_slave(slave);
+ kobject_put(&slave->kobj);
return 0;
}
diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
index 9b8346638f69..fd07561da034 100644
--- a/drivers/net/bonding/bond_sysfs_slave.c
+++ b/drivers/net/bonding/bond_sysfs_slave.c
@@ -121,7 +121,6 @@ static const struct slave_attribute *slave_attrs[] = {
};
#define to_slave_attr(_at) container_of(_at, struct slave_attribute, attr)
-#define to_slave(obj) container_of(obj, struct slave, kobj)
static ssize_t slave_show(struct kobject *kobj,
struct attribute *attr, char *buf)
@@ -132,28 +131,15 @@ static ssize_t slave_show(struct kobject *kobj,
return slave_attr->show(slave, buf);
}
-static const struct sysfs_ops slave_sysfs_ops = {
+const struct sysfs_ops slave_sysfs_ops = {
.show = slave_show,
};
-static struct kobj_type slave_ktype = {
-#ifdef CONFIG_SYSFS
- .sysfs_ops = &slave_sysfs_ops,
-#endif
-};
-
int bond_sysfs_slave_add(struct slave *slave)
{
const struct slave_attribute **a;
int err;
- err = kobject_init_and_add(&slave->kobj, &slave_ktype,
- &(slave->dev->dev.kobj), "bonding_slave");
- if (err) {
- kobject_put(&slave->kobj);
- return err;
- }
-
for (a = slave_attrs; *a; ++a) {
err = sysfs_create_file(&slave->kobj, &((*a)->attr));
if (err) {
@@ -171,6 +157,4 @@ void bond_sysfs_slave_del(struct slave *slave)
for (a = slave_attrs; *a; ++a)
sysfs_remove_file(&slave->kobj, &((*a)->attr));
-
- kobject_put(&slave->kobj);
}
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 7d132cc1e584..d9d0ff3b0ad3 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -185,6 +185,11 @@ struct slave {
struct rtnl_link_stats64 slave_stats;
};
+static inline struct slave *to_slave(struct kobject *kobj)
+{
+ return container_of(kobj, struct slave, kobj);
+}
+
struct bond_up_slave {
unsigned int count;
struct rcu_head rcu;
@@ -750,6 +755,9 @@ extern struct bond_parm_tbl ad_select_tbl[];
/* exported from bond_netlink.c */
extern struct rtnl_link_ops bond_link_ops;
+/* exported from bond_sysfs_slave.c */
+extern const struct sysfs_ops slave_sysfs_ops;
+
static inline netdev_tx_t bond_tx_drop(struct net_device *dev, struct sk_buff *skb)
{
atomic_long_inc(&dev->tx_dropped);
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv3] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-20 14:28 ` [PATCHv3] " Jamie Iles
@ 2020-11-20 15:39 ` Greg Kroah-Hartman
2020-11-21 21:09 ` Jakub Kicinski
0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-11-20 15:39 UTC (permalink / raw)
To: Jamie Iles
Cc: netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
On Fri, Nov 20, 2020 at 02:28:27PM +0000, Jamie Iles wrote:
> syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> struct slave device could result in the following splat:
>
> kobject: 'bonding_slave' (00000000cecdd4fe): kobject_release, parent 0000000074ceb2b2 (delayed 1000)
> bond0 (unregistering): (slave bond_slave_1): Releasing backup interface
> ------------[ cut here ]------------
> ODEBUG: free active (active state 0) object type: timer_list hint: workqueue_select_cpu_near kernel/workqueue.c:1549 [inline]
> ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x98 kernel/workqueue.c:1600
> WARNING: CPU: 1 PID: 842 at lib/debugobjects.c:485 debug_print_object+0x180/0x240 lib/debugobjects.c:485
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 842 Comm: kworker/u4:4 Tainted: G S 5.9.0-rc8+ #96
> Hardware name: linux,dummy-virt (DT)
> Workqueue: netns cleanup_net
> Call trace:
> dump_backtrace+0x0/0x4d8 include/linux/bitmap.h:239
> show_stack+0x34/0x48 arch/arm64/kernel/traps.c:142
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x174/0x1f8 lib/dump_stack.c:118
> panic+0x360/0x7a0 kernel/panic.c:231
> __warn+0x244/0x2ec kernel/panic.c:600
> report_bug+0x240/0x398 lib/bug.c:198
> bug_handler+0x50/0xc0 arch/arm64/kernel/traps.c:974
> call_break_hook+0x160/0x1d8 arch/arm64/kernel/debug-monitors.c:322
> brk_handler+0x30/0xc0 arch/arm64/kernel/debug-monitors.c:329
> do_debug_exception+0x184/0x340 arch/arm64/mm/fault.c:864
> el1_dbg+0x48/0xb0 arch/arm64/kernel/entry-common.c:65
> el1_sync_handler+0x170/0x1c8 arch/arm64/kernel/entry-common.c:93
> el1_sync+0x80/0x100 arch/arm64/kernel/entry.S:594
> debug_print_object+0x180/0x240 lib/debugobjects.c:485
> __debug_check_no_obj_freed lib/debugobjects.c:967 [inline]
> debug_check_no_obj_freed+0x200/0x430 lib/debugobjects.c:998
> slab_free_hook mm/slub.c:1536 [inline]
> slab_free_freelist_hook+0x190/0x210 mm/slub.c:1577
> slab_free mm/slub.c:3138 [inline]
> kfree+0x13c/0x460 mm/slub.c:4119
> bond_free_slave+0x8c/0xf8 drivers/net/bonding/bond_main.c:1492
> __bond_release_one+0xe0c/0xec8 drivers/net/bonding/bond_main.c:2190
> bond_slave_netdev_event drivers/net/bonding/bond_main.c:3309 [inline]
> bond_netdev_event+0x8f0/0xa70 drivers/net/bonding/bond_main.c:3420
> notifier_call_chain+0xf0/0x200 kernel/notifier.c:83
> __raw_notifier_call_chain kernel/notifier.c:361 [inline]
> raw_notifier_call_chain+0x44/0x58 kernel/notifier.c:368
> call_netdevice_notifiers_info+0xbc/0x150 net/core/dev.c:2033
> call_netdevice_notifiers_extack net/core/dev.c:2045 [inline]
> call_netdevice_notifiers net/core/dev.c:2059 [inline]
> rollback_registered_many+0x6a4/0xec0 net/core/dev.c:9347
> unregister_netdevice_many.part.0+0x2c/0x1c0 net/core/dev.c:10509
> unregister_netdevice_many net/core/dev.c:10508 [inline]
> default_device_exit_batch+0x294/0x338 net/core/dev.c:10992
> ops_exit_list.isra.0+0xec/0x150 net/core/net_namespace.c:189
> cleanup_net+0x44c/0x888 net/core/net_namespace.c:603
> process_one_work+0x96c/0x18c0 kernel/workqueue.c:2269
> worker_thread+0x3f0/0xc30 kernel/workqueue.c:2415
> kthread+0x390/0x498 kernel/kthread.c:292
> ret_from_fork+0x10/0x18 arch/arm64/kernel/entry.S:925
>
> This is a potential use-after-free if the sysfs nodes are being accessed
> whilst removing the struct slave, so wait for the object destruction to
> complete before freeing the struct slave itself.
>
> Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
> Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Qiushi Wu <wu000273@umn.edu>
> Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> Cc: Veaceslav Falico <vfalico@gmail.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> ---
> v3:
> - have a single object lifecycle in the struct slave and remove the
> explicit deallocation and defer that to the kobject
Nice, it looks like it should have always been done this way!
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv3] bonding: wait for sysfs kobject destruction before freeing struct slave
2020-11-20 15:39 ` Greg Kroah-Hartman
@ 2020-11-21 21:09 ` Jakub Kicinski
0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2020-11-21 21:09 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jamie Iles, netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek
On Fri, 20 Nov 2020 16:39:51 +0100 Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 02:28:27PM +0000, Jamie Iles wrote:
> > syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> > struct slave device could result in the following splat:
> > This is a potential use-after-free if the sysfs nodes are being accessed
> > whilst removing the struct slave, so wait for the object destruction to
> > complete before freeing the struct slave itself.
>
> Nice, it looks like it should have always been done this way!
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Applied to net, thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-21 21:09 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 8:41 [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave Jamie Iles
2020-11-05 12:49 ` Eric Dumazet
2020-11-05 18:11 ` Jamie Iles
2020-11-06 16:17 ` Eric Dumazet
2020-11-13 17:12 ` [PATCHv2] " Jamie Iles
2020-11-17 20:34 ` Jakub Kicinski
2020-11-18 7:58 ` Greg Kroah-Hartman
2020-11-20 14:28 ` [PATCHv3] " Jamie Iles
2020-11-20 15:39 ` Greg Kroah-Hartman
2020-11-21 21:09 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).