[PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[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 <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 2 ++
 net/core/dev.c                  | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d0aade0..592a603 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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) {
diff --git a/net/core/dev.c b/net/core/dev.c
index fc1e289..9ffeda9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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)
 {
-- 
1.8.0

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[Eric Dumazet]

On Wed, 2013-05-22 at 16:42 +0800, dingtianhong wrote:
> 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 <dingtianhong@huawei.com>
> ---
>  drivers/net/bonding/bond_main.c | 2 ++
>  net/core/dev.c                  | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index d0aade0..592a603 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -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()")

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[Sergei Shtylyov]

Hello.

On 22-05-2013 12:42, dingtianhong wrote:

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

    Please specify complete summary line of this commit.

> 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.

> 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 <dingtianhong@huawei.com>

WBR, Sergei

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[dingtianhong]

On 2013/5/22 19:45, Eric Dumazet wrote:
> On Wed, 2013-05-22 at 16:42 +0800, dingtianhong wrote:
>> 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 <dingtianhong@huawei.com>
>> ---
>>  drivers/net/bonding/bond_main.c | 2 ++
>>  net/core/dev.c                  | 3 +++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index d0aade0..592a603 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -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()")
> 
> 

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.


> 
> .
> 

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[dingtianhong]

On 2013/5/23 2:30, Sergei Shtylyov wrote:
> Hello.
> 
> On 22-05-2013 12:42, dingtianhong wrote:
> 
>> 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.

>> 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.
> 
>> 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 <dingtianhong@huawei.com>
> 
> WBR, Sergei
> 
> 
> 

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[Eric Dumazet]

On Thu, 2013-05-23 at 09:41 +0800, dingtianhong wrote:

> 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.

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[dingtianhong]

On 2013/5/23 9:52, Eric Dumazet wrote:
> On Thu, 2013-05-23 at 09:41 +0800, dingtianhong wrote:
> 
>> 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.
> 
> 
ok, I agree with the views of yours, it is no way to access the rx_handler_data after that.

> 
> 
> 

Re: [PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[Eric Dumazet]

On Thu, 2013-05-23 at 09:57 +0800, dingtianhong wrote:
> commit 00cfec3 (net: add a synchronize_net() in netdev_rx_handler_unregister())
> 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 bonding release met the problem.
> 
> the commit fcd99434f (bonding: get netdev_rx_handler_unregister out of locks)
> fix the bug in bond release, but there is no action to guarantee the
> rx_handler_data is NULL when bond release, so add synchronize_net() behind
> netdev_rx_handler_unregister() to guarantee it.
> 
> This patch adds more comments to netdev_rx_handler_unregister(), as its more
> reasonable.
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/net/bonding/bond_main.c | 2 ++
>  net/core/dev.c                  | 3 +++
>  2 files changed, 5 insertions(+)


I NACK this patch. This makes absolutely no sense to me.

[PATCH] bonding: add synchronize_net() after netdev_rx_handler_unregister

[dingtianhong]

commit 00cfec3 (net: add a synchronize_net() in netdev_rx_handler_unregister())
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 bonding release met the problem.

the commit fcd99434f (bonding: get netdev_rx_handler_unregister out of locks)
fix the bug in bond release, but there is no action to guarantee the
rx_handler_data is NULL when bond release, so add synchronize_net() behind
netdev_rx_handler_unregister() to guarantee it.

This patch adds more comments to netdev_rx_handler_unregister(), as its more
reasonable.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 2 ++
 net/core/dev.c                  | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d0aade0..592a603 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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) {
diff --git a/net/core/dev.c b/net/core/dev.c
index fc1e289..9ffeda9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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)
 {
-- 
1.8.0