bonding: add synchronize_net() after netdev_rx_handler_unregister

PATCH In review dingtianhong 3 participants 7 messages May 22, 2013 – May 23, 2013

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

dingtianhong

author

2013-05-22 08:44 UTC #0 lore raw

PATCH 1/1 bonding: add synchronize_net() after netdev_rx_handler_unregister

commit 00cfec3748 (net: add a synchronize_net() ...) add a synchronize_net() in netdev_rx_handler_unregister() to guarantee the rx_handler is NULL when rx_handler_data is a non NULL in rcu_read_lock().

so the caller should not use netdev_rx_handler_unregister in atomic as it may schedule and sleep, the commit fcd99434f fix the bug in bond release, but the problem is no action to guarantee the rx_handler_data is NULL when bond release, so add synchronize_net() and fix it.

This patch adds more comments to netdev_rx_handler_unregister().

Signed-off-by Ding Tianhong <[email protected]>

---
 drivers/net/bonding/bond_main.c | 2 ++
 net/core/dev.c                  | 3 +++
 2 files changed, 5 insertions(+)

drivers/net/bonding/bond_main.c +2 -0

index d0aade0..592a603 100644
@@ -2007,6 +2007,8 @@ static int __bond_release_one(struct net_device *bond_dev,
 	 * for this slave anymore.
 	 */
 	netdev_rx_handler_unregister(slave_dev);
+	synchronize_net();
+
 	write_lock_bh(&bond->lock);
 
 	if (!all && !bond->params.fail_over_mac) {

net/core/dev.c +3 -0

index fc1e289..9ffeda9 100644
@@ -3344,6 +3344,9 @@ EXPORT_SYMBOL_GPL(netdev_rx_handler_register);
  *	Unregister a receive handler from a device.
  *
  *	The caller must hold the rtnl_mutex.
+ *
+ *	The function should only be called outside the atomic as it
+ *	might sleep and schedule.
  */
 void netdev_rx_handler_unregister(struct net_device *dev)
 {

Signature · 1 line

1.8.0

Eric Dumazet

2013-05-22 11:45 UTC #1 lore raw

Quoted 22 lines · dingtianhong

commit 00cfec3748 (net: add a synchronize_net() ...) add a synchronize_net() in netdev_rx_handler_unregister() to guarantee the rx_handler is NULL when rx_handler_data is a non NULL in rcu_read_lock().
so the caller should not use netdev_rx_handler_unregister in atomic as it may schedule and sleep, the commit fcd99434f fix the bug in bond release, but the problem is no action to guarantee the rx_handler_data is NULL when bond release, so add synchronize_net() and fix it.
This patch adds more comments to netdev_rx_handler_unregister().
Signed-off-by Ding Tianhong <[email protected]>
---
 drivers/net/bonding/bond_main.c | 2 ++
 net/core/dev.c                  | 3 +++
 2 files changed, 5 insertions(+)
drivers/net/bonding/bond_main.c +0 -0
index d0aade0..592a603 100644

drivers/net/bonding/bond_main.c static int __bond_release_one(struct net_device *bond_dev, · line 2013

Quoted 7 lines

drivers/net/bonding/bond_main.c +2 -0

@@ -2007,6 +2007,8 @@ static int __bond_release_one(struct net_device *bond_dev,
 	 * for this slave anymore.
 	 */
 	netdev_rx_handler_unregister(slave_dev);
+	synchronize_net();
+
 	write_lock_bh(&bond->lock);

Patch is not needed.

This was discussed on netdev, please read the archives.

Its even documented in netdev_rx_handler_unregister(), could you please read again commit [1]

What about you describe the problems you really noticed ?

[1] commit
00cfec37484761a44a3b6f4675a54caa618210ae
("net: add a synchronize_net() in netdev_rx_handler_unregister()")

Sergei Shtylyov

2013-05-22 18:30 UTC #5 lore raw

In reply to dingtianhong

Hello.

Quoted 1 line · dingtianhong

commit 00cfec3748 (net: add a synchronize_net() ...) add a synchronize_net()

Please specify complete summary line of this commit.

Quoted 5 lines

in netdev_rx_handler_unregister() to guarantee the rx_handler is NULL when rx_handler_data is a non NULL in rcu_read_lock().
so the caller should not use netdev_rx_handler_unregister in atomic as it may schedule and sleep, the commit fcd99434f

You forgot to specify the summary line of this commit.

Quoted 7 lines

fix the bug in bond release, but the
problem is no action to guarantee the rx_handler_data is NULL when bond release, so add synchronize_net() and fix it.
This patch adds more comments to netdev_rx_handler_unregister().
Signed-off-by Ding Tianhong <[email protected]>

WBR, Sergei

dingtianhong

author

2013-05-23 01:42 UTC #2 lore raw

In reply to Eric Dumazet

Quoted 35 lines · Eric Dumazet

Quoted 22 lines · dingtianhong
commit 00cfec3748 (net: add a synchronize_net() ...) add a synchronize_net() in netdev_rx_handler_unregister() to guarantee the rx_handler is NULL when rx_handler_data is a non NULL in rcu_read_lock().
so the caller should not use netdev_rx_handler_unregister in atomic as it may schedule and sleep, the commit fcd99434f fix the bug in bond release, but the problem is no action to guarantee the rx_handler_data is NULL when bond release, so add synchronize_net() and fix it.
This patch adds more comments to netdev_rx_handler_unregister().
Signed-off-by Ding Tianhong <[email protected]>
---
 drivers/net/bonding/bond_main.c | 2 ++
 net/core/dev.c                  | 3 +++
 2 files changed, 5 insertions(+)
drivers/net/bonding/bond_main.c +0 -0
index d0aade0..592a603 100644
drivers/net/bonding/bond_main.c static int __bond_release_one(struct net_device *bond_dev, · line 2013
Quoted 7 lines
drivers/net/bonding/bond_main.c +2 -0
@@ -2007,6 +2007,8 @@ static int __bond_release_one(struct net_device *bond_dev,
 	 * for this slave anymore.
 	 */
 	netdev_rx_handler_unregister(slave_dev);
+	synchronize_net();
+
 	write_lock_bh(&bond->lock);
 
Patch is not needed.
This was discussed on netdev, please read the archives.

Quoted 10 lines

Its even documented in netdev_rx_handler_unregister(), could you please read again commit [1]
What about you describe the problems you really noticed ?
[1] commit
00cfec37484761a44a3b6f4675a54caa618210ae
("net: add a synchronize_net() in netdev_rx_handler_unregister()")

I totally understand the commit [1] and know the problem that you met at that time, but its not a net core problem, its drivers problem, the function synchronize_net() need to follow netdev_rx_handler_unregister() even though netdev_rx_handler_unregister() has its own synchronize_net(), the commit [2] fcd99434f fix drivers problem follow your opinion, but miss synchronize_net(),so add it.

Quoted 3 lines

.

dingtianhong

author

2013-05-23 01:47 UTC #6 lore raw

In reply to Sergei Shtylyov

Quoted 8 lines · Sergei Shtylyov

Hello.
Quoted 1 line · dingtianhong
commit 00cfec3748 (net: add a synchronize_net() ...) add a synchronize_net()
Please specify complete summary line of this commit.

ok, I will modify and send again, just tell me your opinion, thanks.

Quoted 20 lines

Quoted 5 lines
in netdev_rx_handler_unregister() to guarantee the rx_handler is NULL when rx_handler_data is a non NULL in rcu_read_lock().
so the caller should not use netdev_rx_handler_unregister in atomic as it may schedule and sleep, the commit fcd99434f
You forgot to specify the summary line of this commit.
Quoted 7 lines
fix the bug in bond release, but the
problem is no action to guarantee the rx_handler_data is NULL when bond release, so add synchronize_net() and fix it.
This patch adds more comments to netdev_rx_handler_unregister().
Signed-off-by Ding Tianhong <[email protected]>
WBR, Sergei

Eric Dumazet

2013-05-23 01:52 UTC #3 lore raw

In reply to dingtianhong

Quoted 6 lines · dingtianhong

I totally understand the commit [1] and know the problem that you met at that time, but its not a net core problem, its drivers problem, the function synchronize_net() need to follow netdev_rx_handler_unregister() even though netdev_rx_handler_unregister() has its own synchronize_net(), the commit [2] fcd99434f fix drivers problem follow your opinion, but miss synchronize_net(),so add it.

No, driver is fine.

You absolutely not explained why you believe this is needed.

After netdev_rx_handler_unregister(), no packet will be delivered to the bonding driver.

dingtianhong

author

2013-05-23 07:51 UTC #4 lore raw

Quoted 9 lines · Eric Dumazet

Quoted 6 lines · dingtianhong
I totally understand the commit [1] and know the problem that you met at that time, but its not a net core problem, its drivers problem, the function synchronize_net() need to follow netdev_rx_handler_unregister() even though netdev_rx_handler_unregister() has its own synchronize_net(), the commit [2] fcd99434f fix drivers problem follow your opinion, but miss synchronize_net(),so add it.

Quoted 8 lines

No, driver is fine.
You absolutely not explained why you believe this is needed.
After netdev_rx_handler_unregister(), no packet will be delivered to the bonding driver.

ok, I agree with the views of yours, it is no way to access the rx_handler_data after that.

Quoted 3 lines