bonding: fix error handling if slave is busy

The bonding device can cause kernel panic in the enslave error handling.

If slave device already has a receive handler registered, then the
error unwind does not clear the new entry out of the slave list.
This ends up leaving a reference to freed memory in the bond
device slave linked list.

The following is a simple example:
# modprobe dummy
# ip li add dummy0-1 link dummy0 type macvlan
# modprobe bonding
# echo +dummy0 >/sys/class/net/bond0/bonding/slaves
# ip -s li show dev bond0

This returns with -EBUSY, but the bonding device has bogus entry in
the slave list, and will panic on next operation that gets statistics
from bond0.

The fix is to detach the slave (which removes it from the list)
in the unwind path.


Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Patch is against net-next but should be applied to net (3.2), and
stable (3.1 and 3.0).

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Le 30/12/2011 23:40, Stephen Hemminger a écrit :
> The bonding device can cause kernel panic in the enslave error handling.
>
> If slave device already has a receive handler registered, then the
> error unwind does not clear the new entry out of the slave list.
> This ends up leaving a reference to freed memory in the bond
> device slave linked list.
>
> The following is a simple example:
> # modprobe dummy
> # ip li add dummy0-1 link dummy0 type macvlan
> # modprobe bonding
> # echo +dummy0>/sys/class/net/bond0/bonding/slaves
> # ip -s li show dev bond0
>
> This returns with -EBUSY, but the bonding device has bogus entry in
> the slave list, and will panic on next operation that gets statistics
> from bond0.
>
> The fix is to detach the slave (which removes it from the list)
> in the unwind path.
>
>
> Signed-off-by: Stephen Hemminger<shemminger@vyatta.com>
>
> ---
> Patch is against net-next but should be applied to net (3.2), and
> stable (3.1 and 3.0).
>
> --- a/drivers/net/bonding/bond_main.c	2011-12-30 14:20:03.171823181 -0800
> +++ b/drivers/net/bonding/bond_main.c	2011-12-30 14:20:20.232020474 -0800
> @@ -1853,6 +1853,9 @@ err_dest_symlinks:
>   	bond_destroy_slave_symlinks(bond_dev, slave_dev);
>
>   err_close:
> +	write_lock_bh(&bond->lock);
> +	bond_detach_slave(bond, new_slave);
> +	write_unlock_bh(&bond->lock);
>   	dev_close(slave_dev);
>
>   err_unset_master:

NAK.

There are three 'goto err_close' before the call to bond_attach_slave. For those three goto, your 
path will call bond_detach_slave without a previous call to bond_attach_slave.

This would at least decrement bond->slave_cnt, without having incremented it before.

Do I miss something ?

	Nicolas.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html