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