| Message ID | 20181018151826.8373-1-dbanerje@akamai.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] netpoll: allow cleanup to be synchronous | expand |
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 <nhorman@tuxdriver.com> > CC: "David S. Miller" <davem@davemloft.net> > Signed-off-by: Debabrata Banerjee <dbanerje@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 > >
> From: Neil Horman <nhorman@tuxdriver.com> > 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 Yes I've tested this on a bond device with vlan stacked on top. -Deb > > > CC: Neil Horman <nhorman@tuxdriver.com> > > CC: "David S. Miller" <davem@davemloft.net> > > Signed-off-by: Debabrata Banerjee <dbanerje@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 > > > >
From: Debabrata Banerjee <dbanerje@akamai.com> Date: Thu, 18 Oct 2018 11:18:26 -0400 > 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. > > CC: Neil Horman <nhorman@tuxdriver.com> > CC: "David S. Miller" <davem@davemloft.net> > Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com> Applied, thank you.
On Fri, Oct 19, 2018 at 08:46:45PM +0000, Banerjee, Debabrata wrote: > > From: Neil Horman <nhorman@tuxdriver.com> > > > 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 > > Yes I've tested this on a bond device with vlan stacked on top. > > -Deb > Cool, thanks Acked-by: Neil Horman <nhorman@tuxdriver.com> > > > > > CC: Neil Horman <nhorman@tuxdriver.com> > > > CC: "David S. Miller" <davem@davemloft.net> > > > Signed-off-by: Debabrata Banerjee <dbanerje@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 > > > > > > >
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)
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. CC: Neil Horman <nhorman@tuxdriver.com> CC: "David S. Miller" <davem@davemloft.net> Signed-off-by: Debabrata Banerjee <dbanerje@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(-)