Message-ID: <CAMArcTWuycr9PMtx-UvjKA=PxvX0VY6whrVweowq03aqoQ163Q@mail.gmail.com>
Date:   Fri, 6 Sep 2019 00:21:54 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, vfalico@...il.com,
        Andy Gospodarek <andy@...yhouse.net>,
        Jiří Pírko <jiri@...nulli.us>,
        sd@...asysnail.net, Roopa Prabhu <roopa@...ulusnetworks.com>,
        saeedm@...lanox.com, manishc@...vell.com, rahulv@...vell.com,
        kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        sashal@...nel.org, hare@...e.de, varun@...lsio.com,
        ubraun@...ux.ibm.com, kgraul@...ux.ibm.com
Subject: Re: [PATCH net 03/11] bonding: split IFF_BONDING into IFF_BONDING and IFF_BONDING_SLAVE

On Thu, 5 Sep 2019 at 04:22, Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
>

Hi Jay,
Thank you for the review!

> Taehee Yoo <ap420073@...il.com> wrote:
>
> >The IFF_BONDING means bonding master or bonding slave device.
> >
> >->ndo_add_slave() sets IFF_BONDING flag and ->ndo_del_slave() removes
> >IFF_BONDING flag.
> >This routine makes a problem in the nesting bonding structure.
> >
> >bond1<--bond2
> >
> >Both bond0 and bond1 are bonding device and these should keep having
> >IFF_BONDING flag until they are removed.
> >But bond1 would lose IFF_BONDING at ->ndo_del_slave because that routine
> >can not check whether the slave device is the bonding type or not.
> >So that this patch splits the IFF_BONDING into theIFF_BONDING and
> >the IFF_BONDING_SLAVE. The IFF_BONDING is bonding master flag and
> >IFF_BONDING_SLAVE is bonding slave flag.
> >
> >Test commands:
> >    ip link add bond0 type bond
> >    ip link add bond1 type bond
> >    ip link set bond1 master bond0
> >    ip link set bond1 nomaster
> >    ip link del bond1 type bond
> >    ip link add bond1 type bond
> >
> >Splat looks like:
> >[  149.201107] proc_dir_entry 'bonding/bond1' already registered
> >[  149.208013] WARNING: CPU: 1 PID: 1308 at fs/proc/generic.c:361 proc_register+0x2a9/0x3e0
> >[  149.208866] Modules linked in: bonding veth openvswitch nsh nf_conncount nf_nat nf_conntrack nf_defrag_ipv4 ip_tables6
> >[  149.208866] CPU: 1 PID: 1308 Comm: ip Not tainted 5.3.0-rc7+ #322
> >[  149.208866] RIP: 0010:proc_register+0x2a9/0x3e0
> >[  149.208866] Code: 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 39 01 00 00 48 8b 04 24 48 89 ea 48 c7 c7 a0 a0 13 89 48 8b b0 0
> >[  149.208866] RSP: 0018:ffff88810df9f098 EFLAGS: 00010286
> >[  149.208866] RAX: dffffc0000000008 RBX: ffff8880b5d3aa50 RCX: ffffffff87cdec92
> >[  149.208866] RDX: 0000000000000001 RSI: 0000000000000008 RDI: ffff888116bf6a8c
> >[  149.208866] RBP: ffff8880b5d3acd3 R08: ffffed1022d7ff71 R09: ffffed1022d7ff71
> >[  149.208866] R10: 0000000000000001 R11: ffffed1022d7ff70 R12: ffff8880b5d3abe8
> >[  149.208866] R13: ffff8880b5d3acd2 R14: dffffc0000000000 R15: ffffed1016ba759a
> >[  149.208866] FS:  00007f4bd1f650c0(0000) GS:ffff888116a00000(0000) knlGS:0000000000000000
> >[  149.208866] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >[  149.208866] CR2: 000055e7ca686118 CR3: 0000000106fd4000 CR4: 00000000001006e0
> >[  149.208866] Call Trace:
> >[  149.208866]  proc_create_seq_private+0xb3/0xf0
> >[  149.208866]  bond_create_proc_entry+0x1b3/0x3f0 [bonding]
> >[  149.208866]  bond_netdev_event+0x433/0x970 [bonding]
> >[  149.208866]  ? __module_text_address+0x13/0x140
> >[  149.208866]  notifier_call_chain+0x90/0x160
> >[  149.208866]  register_netdevice+0x9b3/0xd70
> >[  149.208866]  ? alloc_netdev_mqs+0x854/0xc10
> >[  149.208866]  ? netdev_change_features+0xa0/0xa0
> >[  149.208866]  ? rtnl_create_link+0x2ed/0xad0
> >[  149.208866]  bond_newlink+0x2a/0x60 [bonding]
> >[  149.208866]  __rtnl_newlink+0xb75/0x1180
> >[  ... ]
> >
> >Fixes: 0b680e753724 ("[PATCH] bonding: Add priv_flag to avoid event mishandling")
>
>         I'm not sure this Fixes is technically correct, as I don't think
> nesting bonds has induced an oops since 2006.  I don't think nesting
> bonds really does anything useful, but it's been allowed for years (but
> has been broken on and off all that time) so I'm a bit leery of simply
> disallowing nesting of bonds for fear it would break something already
> in use.
>
>         In any event, it would be desirable if this fix could be changed
> to not need a new priv_flag, as this patch would consume the last free
> bit in netdev_priv_flags.  A bond master device that is also a slave
> should have IFF_MASTER set in dev->flags, which could be tested at
> removal time to avoid clearing IFF_BONDING.
>

I have been testing another way that doesn't add a new flag and that
just checks IFF_MASTER and IFF_BONDING when an interface is being deleted.
I think it is simple and works well. so I will send a v2 patch after
some more tests.
Thank you!


>         -J
>
> >Signed-off-by: Taehee Yoo <ap420073@...il.com>
> >---
> > drivers/net/bonding/bond_main.c                     | 13 +++++--------
> > .../net/ethernet/qlogic/netxen/netxen_nic_main.c    |  2 +-
> > drivers/net/hyperv/netvsc_drv.c                     |  3 +--
> > drivers/scsi/fcoe/fcoe.c                            |  2 +-
> > drivers/target/iscsi/cxgbit/cxgbit_cm.c             |  2 +-
> > include/linux/netdevice.h                           |  9 ++++++---
> > 6 files changed, 15 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 931d9d935686..abd008c31c9a 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1560,7 +1560,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> >               goto err_restore_mac;
> >       }
> >
> >-      slave_dev->priv_flags |= IFF_BONDING;
> >+      slave_dev->priv_flags |= IFF_BONDING_SLAVE;
> >       /* initialize slave stats */
> >       dev_get_stats(new_slave->dev, &new_slave->slave_stats);
> >
> >@@ -1816,7 +1816,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> >       slave_disable_netpoll(new_slave);
> >
> > err_close:
> >-      slave_dev->priv_flags &= ~IFF_BONDING;
> >+      slave_dev->priv_flags &= ~IFF_BONDING_SLAVE;
> >       dev_close(slave_dev);
> >
> > err_restore_mac:
> >@@ -2017,7 +2017,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> >       else
> >               dev_set_mtu(slave_dev, slave->original_mtu);
> >
> >-      slave_dev->priv_flags &= ~IFF_BONDING;
> >+      slave_dev->priv_flags &= ~IFF_BONDING_SLAVE;
> >
> >       bond_free_slave(slave);
> >
> >@@ -3221,10 +3221,7 @@ static int bond_netdev_event(struct notifier_block *this,
> >       netdev_dbg(event_dev, "%s received %s\n",
> >                  __func__, netdev_cmd_to_name(event));
> >
> >-      if (!(event_dev->priv_flags & IFF_BONDING))
> >-              return NOTIFY_DONE;
> >-
> >-      if (event_dev->flags & IFF_MASTER) {
> >+      if (netif_is_bond_master(event_dev)) {
> >               int ret;
> >
> >               ret = bond_master_netdev_event(event, event_dev);
> >@@ -3232,7 +3229,7 @@ static int bond_netdev_event(struct notifier_block *this,
> >                       return ret;
> >       }
> >
> >-      if (event_dev->flags & IFF_SLAVE)
> >+      if (netif_is_bond_slave(event_dev))
> >               return bond_slave_netdev_event(event, event_dev);
> >
> >       return NOTIFY_DONE;
> >diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
> >index 58e2eaf77014..5e0389ba1f13 100644
> >--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
> >+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
> >@@ -3340,7 +3340,7 @@ static void netxen_config_master(struct net_device *dev, unsigned long event)
> >        * released and is dev_close()ed in bond_release()
> >        * just before IFF_BONDING is stripped.
> >        */
> >-      if (!master && dev->priv_flags & IFF_BONDING)
> >+      if (!master && netif_is_bond_slave(dev))
> >               netxen_free_ip_list(adapter, true);
> > }
> >
> >diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> >index e8fce6d715ef..6831202d9bcb 100644
> >--- a/drivers/net/hyperv/netvsc_drv.c
> >+++ b/drivers/net/hyperv/netvsc_drv.c
> >@@ -2439,8 +2439,7 @@ static int netvsc_netdev_event(struct notifier_block *this,
> >               return NOTIFY_DONE;
> >
> >       /* Avoid Bonding master dev with same MAC registering as VF */
> >-      if ((event_dev->priv_flags & IFF_BONDING) &&
> >-          (event_dev->flags & IFF_MASTER))
> >+      if (netif_is_bond_master(event_dev))
> >               return NOTIFY_DONE;
> >
> >       switch (event) {
> >diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> >index 00dd47bcbb1e..750a6540eb9d 100644
> >--- a/drivers/scsi/fcoe/fcoe.c
> >+++ b/drivers/scsi/fcoe/fcoe.c
> >@@ -307,7 +307,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
> >       }
> >
> >       /* Do not support for bonding device */
> >-      if (netdev->priv_flags & IFF_BONDING && netdev->flags & IFF_MASTER) {
> >+      if (netif_is_bond_master(netdev)) {
> >               FCOE_NETDEV_DBG(netdev, "Bonded interfaces not supported\n");
> >               return -EOPNOTSUPP;
> >       }
> >diff --git a/drivers/target/iscsi/cxgbit/cxgbit_cm.c b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> >index c70caf4ea490..16c8cae333b2 100644
> >--- a/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> >+++ b/drivers/target/iscsi/cxgbit/cxgbit_cm.c
> >@@ -247,7 +247,7 @@ struct cxgbit_device *cxgbit_find_device(struct net_device *ndev, u8 *port_id)
> >
> > static struct net_device *cxgbit_get_real_dev(struct net_device *ndev)
> > {
> >-      if (ndev->priv_flags & IFF_BONDING) {
> >+      if (netif_is_bond_master(ndev) || netif_is_bond_slave(ndev)) {
> >               pr_err("Bond devices are not supported. Interface:%s\n",
> >                      ndev->name);
> >               return NULL;
> >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >index 5bb5756129af..a2c47f43e54b 100644
> >--- a/include/linux/netdevice.h
> >+++ b/include/linux/netdevice.h
> >@@ -1441,7 +1441,7 @@ struct net_device_ops {
> >  *
> >  * @IFF_802_1Q_VLAN: 802.1Q VLAN device
> >  * @IFF_EBRIDGE: Ethernet bridging device
> >- * @IFF_BONDING: bonding master or slave
> >+ * @IFF_BONDING: bonding master
> >  * @IFF_ISATAP: ISATAP interface (RFC4214)
> >  * @IFF_WAN_HDLC: WAN HDLC device
> >  * @IFF_XMIT_DST_RELEASE: dev_hard_start_xmit() is allowed to
> >@@ -1474,6 +1474,7 @@ struct net_device_ops {
> >  * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> >  * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
> >  * @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
> >+ * @IFF_BONDING_SLAVE: bonding slave
> >  */
> > enum netdev_priv_flags {
> >       IFF_802_1Q_VLAN                 = 1<<0,
> >@@ -1507,6 +1508,7 @@ enum netdev_priv_flags {
> >       IFF_FAILOVER_SLAVE              = 1<<28,
> >       IFF_L3MDEV_RX_HANDLER           = 1<<29,
> >       IFF_LIVE_RENAME_OK              = 1<<30,
> >+      IFF_BONDING_SLAVE               = 1<<31,
> > };
> >
> > #define IFF_802_1Q_VLAN                       IFF_802_1Q_VLAN
> >@@ -1539,6 +1541,7 @@ enum netdev_priv_flags {
> > #define IFF_FAILOVER_SLAVE            IFF_FAILOVER_SLAVE
> > #define IFF_L3MDEV_RX_HANDLER         IFF_L3MDEV_RX_HANDLER
> > #define IFF_LIVE_RENAME_OK            IFF_LIVE_RENAME_OK
> >+#define IFF_BONDING_SLAVE             IFF_BONDING_SLAVE
> >
> > /**
> >  *    struct net_device - The DEVICE structure.
> >@@ -4569,12 +4572,12 @@ static inline bool netif_is_macvlan_port(const struct net_device *dev)
> >
> > static inline bool netif_is_bond_master(const struct net_device *dev)
> > {
> >-      return dev->flags & IFF_MASTER && dev->priv_flags & IFF_BONDING;
> >+      return dev->priv_flags & IFF_BONDING;
> > }
> >
> > static inline bool netif_is_bond_slave(const struct net_device *dev)
> > {
> >-      return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_BONDING;
> >+      return dev->priv_flags & IFF_BONDING_SLAVE;
> > }
> >
> > static inline bool netif_supports_nofcs(struct net_device *dev)
> >--
> >2.17.1
> >
>
> ---
>         -Jay Vosburgh, jay.vosburgh@...onical.com

Powered by blists - more mailing lists