On Thu, Oct 18, 2018 at 11:18:26AM -0400, Debabrata Banerjee wrote:
> This fixes a problem introduced by:
> commit 2cde6acd49da ("netpoll: Fix __netpoll_rcu_free so that it can hold the
> rtnl lock")
>
> When using netconsole on a bond, __netpoll_cleanup can asynchronously
> recurse multiple times, each __netpoll_free_async call can result in
> more __netpoll_free_async's. This means there is now a race between
> cleanup_work queues on multiple netpoll_info's on multiple devices and
> the configuration of a new netpoll. For example if a netconsole is set
> to enable 0, reconfigured, and enable 1 immediately, this netconsole
> will likely not work.
>
> Given the reason for __netpoll_free_async is it can be called when rtnl
> is not locked, if it is locked, we should be able to execute
> synchronously. It appears to be locked everywhere it's called from.
>
> Generalize the design pattern from the teaming driver for current
> callers of __netpoll_free_async.
>
I presume you've tested this with some of the stacked devices? I think I'm ok
with this change, but I'd like confirmation that its worked.
Neil
> CC: Neil Horman <nhor...@tuxdriver.com>
> CC: "David S. Miller" <da...@davemloft.net>
> Signed-off-by: Debabrata Banerjee <dbane...@akamai.com>
> ---
> drivers/net/bonding/bond_main.c | 3 ++-
> drivers/net/macvlan.c | 2 +-
> drivers/net/team/team.c | 5 +----
> include/linux/netpoll.h | 4 +---
> net/8021q/vlan_dev.c | 3 +--
> net/bridge/br_device.c | 2 +-
> net/core/netpoll.c | 20 +++++---------------
> net/dsa/slave.c | 2 +-
> 8 files changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index ee28ec9e0aba..ffa37adb7681 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -963,7 +963,8 @@ static inline void slave_disable_netpoll(struct slave
> *slave)
> return;
>
> slave->np = NULL;
> - __netpoll_free_async(np);
> +
> + __netpoll_free(np);
> }
>
> static void bond_poll_controller(struct net_device *bond_dev)
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index cfda146f3b3b..fc8d5f1ee1ad 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1077,7 +1077,7 @@ static void macvlan_dev_netpoll_cleanup(struct
> net_device *dev)
>
> vlan->netpoll = NULL;
>
> - __netpoll_free_async(netpoll);
> + __netpoll_free(netpoll);
> }
> #endif /* CONFIG_NET_POLL_CONTROLLER */
>
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index d887016e54b6..db633ae9f784 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -1104,10 +1104,7 @@ static void team_port_disable_netpoll(struct team_port
> *port)
> return;
> port->np = NULL;
>
> - /* Wait for transmitting packets to finish before freeing. */
> - synchronize_rcu_bh();
> - __netpoll_cleanup(np);
> - kfree(np);
> + __netpoll_free(np);
> }
> #else
> static int team_port_enable_netpoll(struct team_port *port)
> diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> index 3ef82d3a78db..676f1ff161a9 100644
> --- a/include/linux/netpoll.h
> +++ b/include/linux/netpoll.h
> @@ -31,8 +31,6 @@ struct netpoll {
> bool ipv6;
> u16 local_port, remote_port;
> u8 remote_mac[ETH_ALEN];
> -
> - struct work_struct cleanup_work;
> };
>
> struct netpoll_info {
> @@ -63,7 +61,7 @@ int netpoll_parse_options(struct netpoll *np, char *opt);
> int __netpoll_setup(struct netpoll *np, struct net_device *ndev);
> int netpoll_setup(struct netpoll *np);
> void __netpoll_cleanup(struct netpoll *np);
> -void __netpoll_free_async(struct netpoll *np);
> +void __netpoll_free(struct netpoll *np);
> void netpoll_cleanup(struct netpoll *np);
> void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
> struct net_device *dev);
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 546af0e73ac3..ff720f1ebf73 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -756,8 +756,7 @@ static void vlan_dev_netpoll_cleanup(struct net_device
> *dev)
> return;
>
> vlan->netpoll = NULL;
> -
> - __netpoll_free_async(netpoll);
> + __netpoll_free(netpoll);
> }
> #endif /* CONFIG_NET_POLL_CONTROLLER */
>
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index e053a4e43758..c6abf927f0c9 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -344,7 +344,7 @@ void br_netpoll_disable(struct net_bridge_port *p)
>
> p->np = NULL;
>
> - __netpoll_free_async(np);
> + __netpoll_free(np);
> }
>
> #endif
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index de1d1ba92f2d..6ac71624ead4 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -591,7 +591,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device
> *ndev)
>
> np->dev = ndev;
> strlcpy(np->dev_name, ndev->name, IFNAMSIZ);
> - INIT_WORK(&np->cleanup_work, netpoll_async_cleanup);
>
> if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
> np_err(np, "%s doesn't support polling, aborting\n",
> @@ -790,10 +789,6 @@ void __netpoll_cleanup(struct netpoll *np)
> {
> struct netpoll_info *npinfo;
>
> - /* rtnl_dereference would be preferable here but
> - * rcu_cleanup_netpoll path can put us in here safely without
> - * holding the rtnl, so plain rcu_dereference it is
> - */
> npinfo = rtnl_dereference(np->dev->npinfo);
> if (!npinfo)
> return;
> @@ -814,21 +809,16 @@ void __netpoll_cleanup(struct netpoll *np)
> }
> EXPORT_SYMBOL_GPL(__netpoll_cleanup);
>
> -static void netpoll_async_cleanup(struct work_struct *work)
> +void __netpoll_free(struct netpoll *np)
> {
> - struct netpoll *np = container_of(work, struct netpoll, cleanup_work);
> + ASSERT_RTNL();
>
> - rtnl_lock();
> + /* Wait for transmitting packets to finish before freeing. */
> + synchronize_rcu_bh();
> __netpoll_cleanup(np);
> - rtnl_unlock();
> kfree(np);
> }
> -
> -void __netpoll_free_async(struct netpoll *np)
> -{
> - schedule_work(&np->cleanup_work);
> -}
> -EXPORT_SYMBOL_GPL(__netpoll_free_async);
> +EXPORT_SYMBOL_GPL(__netpoll_free);
>
> void netpoll_cleanup(struct netpoll *np)
> {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 3f840b6eea69..3679e13b2ead 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -722,7 +722,7 @@ static void dsa_slave_netpoll_cleanup(struct net_device
> *dev)
>
> p->netpoll = NULL;
>
> - __netpoll_free_async(netpoll);
> + __netpoll_free(netpoll);
> }
>
> static void dsa_slave_poll_controller(struct net_device *dev)
> --
> 2.19.1
>
>
