On 9/1/16 11:18 PM, Mahesh Bandewar wrote:
> From: Mahesh Bandewar <mahe...@google.com>
>
> Following few steps will crash kernel -
>
> (a) Create bonding master
> > modprobe bonding miimon=50
> (b) Create macvlan bridge on eth2
> > ip link add link eth2 dev mvl0 address aa:0:0:0:0:01 \
> type macvlan
> (c) Now try adding eth2 into the bond
> > echo +eth2 > /sys/class/net/bond0/bonding/slaves
> <crash>
>
> Bonding does lots of things before checking if the device enslaved is
> busy or not.
>
> In this case when the notifier call-chain sends notifications, the
> bond_netdev_event() assumes that the rx_handler /rx_handler_data is
> registered while the bond_enslave() hasn't progressed far enough to
> register rx_handler for the new slave.
>
> This patch adds a rx_handler check that can be performed right at the
> beginning of the enslave code to avoid getting into this situation.
>
> Signed-off-by: Mahesh Bandewar <mahe...@google.com>
> ---
> drivers/net/bonding/bond_main.c | 7 ++++---
> include/linux/netdevice.h | 1 +
> net/core/dev.c | 16 ++++++++++++++++
> 3 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 217e8da0628c..9599ed6f1213 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1341,9 +1341,10 @@ int bond_enslave(struct net_device *bond_dev, struct
> net_device *slave_dev)
> slave_dev->name);
> }
>
> - /* already enslaved */
> - if (slave_dev->flags & IFF_SLAVE) {
> - netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
> + /* already in-use? */
> + if (netdev_is_rx_handler_busy(slave_dev)) {
> + netdev_err(bond_dev,
> + "Error: Device is in use and cannot be enslaved\n");
> return -EBUSY;
> }
>
This check duplicates what netdev_rx_handler_register does. Why not move the
call to netdev_rx_handler_register here and then call unregister on failure
paths?
