[PATCH] vxlan: remove the redundant gro_cells_destroy() calling.

[PATCH] vxlan: remove the redundant gro_cells_destroy() calling.

[Zhiqiang Liu]

From: "Suanming.Mou" <mousuanming@huawei.com>

With ad6c9986bcb6, GRO cells will be destroyed in vxlan_uninit.

Fixes: ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link delete")

Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
 drivers/net/vxlan.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 077f1b9f2761..d76dfed8d9bb 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4335,10 +4335,8 @@ static void vxlan_destroy_tunnels(struct net *net, struct list_head *head)
 		/* If vxlan->dev is in the same netns, it has already been added
 		 * to the list by the previous loop.
 		 */
-		if (!net_eq(dev_net(vxlan->dev), net)) {
-			gro_cells_destroy(&vxlan->gro_cells);
+		if (!net_eq(dev_net(vxlan->dev), net))
 			unregister_netdevice_queue(vxlan->dev, head);
-		}
 	}

 	for (h = 0; h < PORT_HASH_SIZE; ++h)
-- 
1.7.12.4

Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling.

[Stefano Brivio]

On Fri, 15 Mar 2019 18:06:45 +0800
Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:

> From: "Suanming.Mou" <mousuanming@huawei.com>
> 
> With ad6c9986bcb6, GRO cells will be destroyed in vxlan_uninit.

Thanks for cleaning this up.

I think it would be nice if you could actually explain in the commit
message why this makes the call in vxlan_destroy_tunnels() redundant.

> Fixes: ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link delete")

I'm not sure a Fixes: tag is appropriate here (and also if this
shouldn't be targeted for net-next) -- in the end, gro_cells_destroy()
there would just return:

        if (!gcells->cells)
                return;

> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>

Either way,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling.

[Zhiqiang Liu]

> On Fri, 15 Mar 2019 18:06:45 +0800
> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> 
>> From: "Suanming.Mou" <mousuanming@huawei.com>
>>
>> With ad6c9986bcb6, GRO cells will be destroyed in vxlan_uninit.
> 
> Thanks for cleaning this up.
> 
> I think it would be nice if you could actually explain in the commit
> message why this makes the call in vxlan_destroy_tunnels() redundant.
> 
Thanks for your reply. Actually, the patch is a cleanup as you said.
In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after gro_cells_destroy
func. However, in unregister_netdevice_queue func, the gro_cells_destroy func will also call
the gro_cells_destroy func as the following routine:
	unregister_netdevice_many -> rollback_registered_many -> ndo_uninit -> gro_cells_destroy

Fortunately, gro_cells_destroy func will check whether gcells->cells is NULL, so even more than
one call gro_cells_destroy would not cause the memory twice-free problem.

>> Fixes: ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link delete")
> 
> I'm not sure a Fixes: tag is appropriate here (and also if this
> shouldn't be targeted for net-next) -- in the end, gro_cells_destroy()
> there would just return:
> 
>         if (!gcells->cells)
>                 return;
> 
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>

As you said, this is just a cleanup. I will remove the Fixes tag in v2 patch.
I used the ./scripts/get_maintainer.pl to get the maintainers and mail-list, and the return is
given as follows,

[root@localhost linux]# ./scripts/get_maintainer.pl 0001-vxlan-remove-the-redundant-gro_cells_destroy-calling.patch
"David S. Miller" <davem@davemloft.net> (odd fixer:NETWORKING DRIVERS,commit_signer:57/57=100%)
Petr Machata <petrm@mellanox.com> (commit_signer:30/57=53%,authored:27/57=47%,added_lines:649/1160=56%,removed_lines:265/494=54%)
Ido Schimmel <idosch@mellanox.com> (commit_signer:15/57=26%,removed_lines:30/494=6%)
Roopa Prabhu <roopa@cumulusnetworks.com> (commit_signer:11/57=19%,authored:9/57=16%,added_lines:364/1160=31%,removed_lines:156/494=32%)
Sabrina Dubroca <sd@queasysnail.net> (commit_signer:6/57=11%)
Stefano Brivio <sbrivio@redhat.com> (authored:5/57=9%,added_lines:63/1160=5%)
netdev@vger.kernel.org (open list:NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

> 
> Either way,
> 
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> 

Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling.

[Eric Dumazet]

On 03/15/2019 03:06 AM, Zhiqiang Liu wrote:
> From: "Suanming.Mou" <mousuanming@huawei.com>
> 
> With ad6c9986bcb6, GRO cells will be destroyed in vxlan_uninit.
> 
> Fixes: ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link delete")
> 

This is a net-next candidate .

The Fixes: tag is not needed, the cited commit added no bug.

[PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Zhiqiang Liu]

In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
gro_cells_destroy func. However, in unregister_netdevice_queue func, the
gro_cells_destroy func will also call the gro_cells_destroy func as the
following routine:
unregister_netdevice_many() -> rollback_registered_many()
-> ndo_uninit() -> gro_cells_destroy()

Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
---
 drivers/net/vxlan.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 077f1b9f2761..d76dfed8d9bb 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4335,10 +4335,8 @@ static void vxlan_destroy_tunnels(struct net *net, struct list_head *head)
 		/* If vxlan->dev is in the same netns, it has already been added
 		 * to the list by the previous loop.
 		 */
-		if (!net_eq(dev_net(vxlan->dev), net)) {
-			gro_cells_destroy(&vxlan->gro_cells);
+		if (!net_eq(dev_net(vxlan->dev), net))
 			unregister_netdevice_queue(vxlan->dev, head);
-		}
 	}

 	for (h = 0; h < PORT_HASH_SIZE; ++h)
-- 
1.7.12.4

Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling.

[Stefano Brivio]

On Fri, 15 Mar 2019 22:55:52 +0800
Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:

> > On Fri, 15 Mar 2019 18:06:45 +0800
> > Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> >   
> >> From: "Suanming.Mou" <mousuanming@huawei.com>
> >>
> >> With ad6c9986bcb6, GRO cells will be destroyed in vxlan_uninit.  
> > 
> > Thanks for cleaning this up.
> > 
> > I think it would be nice if you could actually explain in the commit
> > message why this makes the call in vxlan_destroy_tunnels() redundant.
> >   
> Thanks for your reply. Actually, the patch is a cleanup as you said.
> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after gro_cells_destroy
> func. However, in unregister_netdevice_queue func, the gro_cells_destroy func will also call
> the gro_cells_destroy func as the following routine:
> 	unregister_netdevice_many -> rollback_registered_many -> ndo_uninit -> gro_cells_destroy

Yes, I think this is exactly what you should add in the commit
message of v2.

> Fortunately, gro_cells_destroy func will check whether gcells->cells is NULL, so even more than
> one call gro_cells_destroy would not cause the memory twice-free problem.
> 
> >> Fixes: ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link delete")  
> > 
> > I'm not sure a Fixes: tag is appropriate here (and also if this
> > shouldn't be targeted for net-next) -- in the end, gro_cells_destroy()
> > there would just return:
> > 
> >         if (!gcells->cells)
> >                 return;
> >   
> >> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>  
> 
> As you said, this is just a cleanup. I will remove the Fixes tag in v2 patch.
> I used the ./scripts/get_maintainer.pl to get the maintainers and mail-list, and the return is
> given as follows,
> 
> [...]

That's correct, you sent the patch to the right addresses. But there
are two trees for networking, net and net-next. See:

	"Q: How do I indicate which tree (net vs. net-next) my patch
	should be in?"

in Documentation/networking/netdev-FAQ.rst (perhaps you should also
read the whole thing while at it). Mind that net-next is currently
closed, so you'll have to wait a bit.

-- 
Stefano

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Stefano Brivio]

On Fri, 15 Mar 2019 23:18:52 +0800
Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:

> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
> gro_cells_destroy func will also call the gro_cells_destroy func as the
> following routine:
> unregister_netdevice_many() -> rollback_registered_many()
> -> ndo_uninit() -> gro_cells_destroy()  
> 
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

NACK, please read my and Eric's comments to v1 -- giving me more than 23
minutes to answer would have been a nice touch as well :)

-- 
Stefano

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Eric Dumazet]

On 03/15/2019 08:28 AM, Stefano Brivio wrote:
> On Fri, 15 Mar 2019 23:18:52 +0800
> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> 
>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
>> gro_cells_destroy func will also call the gro_cells_destroy func as the
>> following routine:
>> unregister_netdevice_many() -> rollback_registered_many()
>> -> ndo_uninit() -> gro_cells_destroy()  
>>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> 
> NACK, please read my and Eric's comments to v1 -- giving me more than 23
> minutes to answer would have been a nice touch as well :)
> 

Sorry for the confusion, I forgot to add the question marks to my sentences.

In fact, this is a bug fix, that we missed in the previous fix.

Technically the bug is older.

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[David Miller]

From: Stefano Brivio <sbrivio@redhat.com>
Date: Fri, 15 Mar 2019 16:28:24 +0100

> On Fri, 15 Mar 2019 23:18:52 +0800
> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> 
>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
>> gro_cells_destroy func will also call the gro_cells_destroy func as the
>> following routine:
>> unregister_netdevice_many() -> rollback_registered_many()
>> -> ndo_uninit() -> gro_cells_destroy()  
>> 
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> 
> NACK, please read my and Eric's comments to v1 -- giving me more than 23
> minutes to answer would have been a nice touch as well :)

Also the net-next tree is CLOSED.

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 Mar 2019 09:06:25 -0700

> 
> 
> On 03/15/2019 08:28 AM, Stefano Brivio wrote:
>> On Fri, 15 Mar 2019 23:18:52 +0800
>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>> 
>>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
>>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
>>> gro_cells_destroy func will also call the gro_cells_destroy func as the
>>> following routine:
>>> unregister_netdevice_many() -> rollback_registered_many()
>>> -> ndo_uninit() -> gro_cells_destroy()  
>>>
>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
>> 
>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
>> minutes to answer would have been a nice touch as well :)
>> 
> 
> Sorry for the confusion, I forgot to add the question marks to my sentences.
> 
> In fact, this is a bug fix, that we missed in the previous fix.
> 
> Technically the bug is older.

Please elaborate.

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Eric Dumazet]

On 03/15/2019 11:02 AM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 15 Mar 2019 09:06:25 -0700
> 
>>
>>
>> On 03/15/2019 08:28 AM, Stefano Brivio wrote:
>>> On Fri, 15 Mar 2019 23:18:52 +0800
>>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>>>
>>>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
>>>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
>>>> gro_cells_destroy func will also call the gro_cells_destroy func as the
>>>> following routine:
>>>> unregister_netdevice_many() -> rollback_registered_many()
>>>> -> ndo_uninit() -> gro_cells_destroy()  
>>>>
>>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
>>>
>>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
>>> minutes to answer would have been a nice touch as well :)
>>>
>>
>> Sorry for the confusion, I forgot to add the question marks to my sentences.
>>
>> In fact, this is a bug fix, that we missed in the previous fix.
>>
>> Technically the bug is older.
> 
> Please elaborate.
> 

Commit ad6c9986bcb62
("vxlan: Fix GRO cells race condition between receive and link delete")

fixed a race condition for the typical case a vxlan device is dismantled from the
current netns.

But if a netns is dismantled, we call vxlan_destroy_tunnels()
to schedule a unregister_netdevice_queue() of all the vxlan tunnels
that are related to this netns.

This means that the gro_cells_destroy() call is done too soon,
for the same reasons explained in above commit .

We need to fully respect the RCU rules, and thus must remove the
gro_cells_destroy() call or risk use after-free.

The bug is day-0 I think.

commit 58ce31cca1ffe057f4744c3f671e3e84606d3d4a
Author: Tom Herbert <tom@herbertland.com>
Date:   Wed Aug 19 17:07:33 2015 -0700

    vxlan: GRO support at tunnel layer

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Stefano Brivio]

On Fri, 15 Mar 2019 11:56:01 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On 03/15/2019 11:02 AM, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Fri, 15 Mar 2019 09:06:25 -0700
> >   
> >>
> >>
> >> On 03/15/2019 08:28 AM, Stefano Brivio wrote:  
> >>> On Fri, 15 Mar 2019 23:18:52 +0800
> >>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> >>>  
> >>>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
> >>>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
> >>>> gro_cells_destroy func will also call the gro_cells_destroy func as the
> >>>> following routine:
> >>>> unregister_netdevice_many() -> rollback_registered_many()  
> >>>> -> ndo_uninit() -> gro_cells_destroy()    
> >>>>
> >>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> >>>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> >>>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>  
> >>>
> >>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
> >>> minutes to answer would have been a nice touch as well :)
> >>>  
> >>
> >> Sorry for the confusion, I forgot to add the question marks to my sentences.
> >>
> >> In fact, this is a bug fix, that we missed in the previous fix.
> >>
> >> Technically the bug is older.  
> > 
> > Please elaborate.
> >   
> 
> Commit ad6c9986bcb62
> ("vxlan: Fix GRO cells race condition between receive and link delete")
> 
> fixed a race condition for the typical case a vxlan device is dismantled from the
> current netns.
> 
> But if a netns is dismantled, we call vxlan_destroy_tunnels()
> to schedule a unregister_netdevice_queue() of all the vxlan tunnels
> that are related to this netns.

Won't that happen via ops_exit_list() only after synchronize_rcu() is
called by cleanup_net(), though? Is there another path I missed?

-- 
Stefano

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Eric Dumazet]

On 03/15/2019 02:08 PM, Stefano Brivio wrote:
> On Fri, 15 Mar 2019 11:56:01 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> On 03/15/2019 11:02 AM, David Miller wrote:
>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date: Fri, 15 Mar 2019 09:06:25 -0700
>>>   
>>>>
>>>>
>>>> On 03/15/2019 08:28 AM, Stefano Brivio wrote:  
>>>>> On Fri, 15 Mar 2019 23:18:52 +0800
>>>>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>>>>>  
>>>>>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
>>>>>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
>>>>>> gro_cells_destroy func will also call the gro_cells_destroy func as the
>>>>>> following routine:
>>>>>> unregister_netdevice_many() -> rollback_registered_many()  
>>>>>> -> ndo_uninit() -> gro_cells_destroy()    
>>>>>>
>>>>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>>>>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>>>>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>  
>>>>>
>>>>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
>>>>> minutes to answer would have been a nice touch as well :)
>>>>>  
>>>>
>>>> Sorry for the confusion, I forgot to add the question marks to my sentences.
>>>>
>>>> In fact, this is a bug fix, that we missed in the previous fix.
>>>>
>>>> Technically the bug is older.  
>>>
>>> Please elaborate.
>>>   
>>
>> Commit ad6c9986bcb62
>> ("vxlan: Fix GRO cells race condition between receive and link delete")
>>
>> fixed a race condition for the typical case a vxlan device is dismantled from the
>> current netns.
>>
>> But if a netns is dismantled, we call vxlan_destroy_tunnels()
>> to schedule a unregister_netdevice_queue() of all the vxlan tunnels
>> that are related to this netns.
> 
> Won't that happen via ops_exit_list() only after synchronize_rcu() is
> called by cleanup_net(), though? Is there another path I missed?

Just look at vxlan_destroy_tunnels().

The call to gro_cells_destroy(&vxlan->gro_cells);
is done _before_ 
unregister_netdevice_queue(vxlan->dev, head);

So packets can still fly, the RCU grace period has not yet started.

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Stefano Brivio]

On Fri, 15 Mar 2019 14:26:10 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On 03/15/2019 02:08 PM, Stefano Brivio wrote:
> > On Fri, 15 Mar 2019 11:56:01 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >   
> >> On 03/15/2019 11:02 AM, David Miller wrote:  
> >>> From: Eric Dumazet <eric.dumazet@gmail.com>
> >>> Date: Fri, 15 Mar 2019 09:06:25 -0700
> >>>     
> >>>>
> >>>>
> >>>> On 03/15/2019 08:28 AM, Stefano Brivio wrote:    
> >>>>> On Fri, 15 Mar 2019 23:18:52 +0800
> >>>>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> >>>>>    
> >>>>>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
> >>>>>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
> >>>>>> gro_cells_destroy func will also call the gro_cells_destroy func as the
> >>>>>> following routine:
> >>>>>> unregister_netdevice_many() -> rollback_registered_many()    
> >>>>>> -> ndo_uninit() -> gro_cells_destroy()      
> >>>>>>
> >>>>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> >>>>>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> >>>>>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>    
> >>>>>
> >>>>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
> >>>>> minutes to answer would have been a nice touch as well :)
> >>>>>    
> >>>>
> >>>> Sorry for the confusion, I forgot to add the question marks to my sentences.
> >>>>
> >>>> In fact, this is a bug fix, that we missed in the previous fix.
> >>>>
> >>>> Technically the bug is older.    
> >>>
> >>> Please elaborate.
> >>>     
> >>
> >> Commit ad6c9986bcb62
> >> ("vxlan: Fix GRO cells race condition between receive and link delete")
> >>
> >> fixed a race condition for the typical case a vxlan device is dismantled from the
> >> current netns.
> >>
> >> But if a netns is dismantled, we call vxlan_destroy_tunnels()
> >> to schedule a unregister_netdevice_queue() of all the vxlan tunnels
> >> that are related to this netns.  
> > 
> > Won't that happen via ops_exit_list() only after synchronize_rcu() is
> > called by cleanup_net(), though? Is there another path I missed?  
> 
> Just look at vxlan_destroy_tunnels().
> 
> The call to gro_cells_destroy(&vxlan->gro_cells);
> is done _before_ 
> unregister_netdevice_queue(vxlan->dev, head);
> 
> So packets can still fly, the RCU grace period has not yet started.

Wait, what... :/ thanks for pointing that out, I guess it was too
obvious for me to notice.

Zhiqiang, could you maybe update the commit message with these two bits
of information (the real issue explained by Eric, and the different
Fixes: tag), and post v3?

This would be an actual fix and not a clean-up, so it doesn't need to
wait for net-next to re-open.

-- 
Stefano

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Zhiqiang Liu]

>>> On 03/15/2019 08:28 AM, Stefano Brivio wrote:
>>>> On Fri, 15 Mar 2019 23:18:52 +0800
>>>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>>>>
>>>>
>>>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
>>>> minutes to answer would have been a nice touch as well :)
>>>>
>>>
>>> Sorry for the confusion, I forgot to add the question marks to my sentences.
>>>
>>> In fact, this is a bug fix, that we missed in the previous fix.
>>>
>>> Technically the bug is older.
>>
>> Please elaborate.
>>
> 
> Commit ad6c9986bcb62
> ("vxlan: Fix GRO cells race condition between receive and link delete")
> 
> fixed a race condition for the typical case a vxlan device is dismantled from the
> current netns.
> 
> But if a netns is dismantled, we call vxlan_destroy_tunnels()
> to schedule a unregister_netdevice_queue() of all the vxlan tunnels
> that are related to this netns.
> 
> This means that the gro_cells_destroy() call is done too soon,
> for the same reasons explained in above commit .
> 
> We need to fully respect the RCU rules, and thus must remove the
> gro_cells_destroy() call or risk use after-free.
> 
> The bug is day-0 I think.
> 
> commit 58ce31cca1ffe057f4744c3f671e3e84606d3d4a
> Author: Tom Herbert <tom@herbertland.com>
> Date:   Wed Aug 19 17:07:33 2015 -0700
> 
>     vxlan: GRO support at tunnel layer
> 

Thank you for pointing out the real issueI will update the commit message and Fixes: tag as your explanation.

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Zhiqiang Liu]

:
> On Fri, 15 Mar 2019 14:26:10 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> On 03/15/2019 02:08 PM, Stefano Brivio wrote:
>>> On Fri, 15 Mar 2019 11:56:01 -0700
>>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>   
>>>> On 03/15/2019 11:02 AM, David Miller wrote:  
>>>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>>> Date: Fri, 15 Mar 2019 09:06:25 -0700
>>>>>     
>>>>>>
>>>>>>
>>>>>> On 03/15/2019 08:28 AM, Stefano Brivio wrote:    
>>>>>>> On Fri, 15 Mar 2019 23:18:52 +0800
>>>>>>> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>>>>>>>    
>>>>>>>> In vxlan_destroy_tunnels func, unregister_netdevice_queue is called after
>>>>>>>> gro_cells_destroy func. However, in unregister_netdevice_queue func, the
>>>>>>>> gro_cells_destroy func will also call the gro_cells_destroy func as the
>>>>>>>> following routine:
>>>>>>>> unregister_netdevice_many() -> rollback_registered_many()    
>>>>>>>> -> ndo_uninit() -> gro_cells_destroy()      
>>>>>>>> 
>>>>>>>
>>>>>>> NACK, please read my and Eric's comments to v1 -- giving me more than 23
>>>>>>> minutes to answer would have been a nice touch as well :)
>>>>>>>    
>>>>>>
>>>>>> Sorry for the confusion, I forgot to add the question marks to my sentences.
>>>>>>
>>>>>> In fact, this is a bug fix, that we missed in the previous fix.
>>>>>>
>>>>>> Technically the bug is older.    
>>>>>
>>>>> Please elaborate.
>>>>>     
>>>>
>>>> Commit ad6c9986bcb62
>>>> ("vxlan: Fix GRO cells race condition between receive and link delete")
>>>>
>>>> fixed a race condition for the typical case a vxlan device is dismantled from the
>>>> current netns.
>>>>
>>>> But if a netns is dismantled, we call vxlan_destroy_tunnels()
>>>> to schedule a unregister_netdevice_queue() of all the vxlan tunnels
>>>> that are related to this netns.  
>>>
>>> Won't that happen via ops_exit_list() only after synchronize_rcu() is
>>> called by cleanup_net(), though? Is there another path I missed?  
>>
>> Just look at vxlan_destroy_tunnels().
>>
>> The call to gro_cells_destroy(&vxlan->gro_cells);
>> is done _before_ 
>> unregister_netdevice_queue(vxlan->dev, head);
>>
>> So packets can still fly, the RCU grace period has not yet started.
> 
> Wait, what... :/ thanks for pointing that out, I guess it was too
> obvious for me to notice.
> 
> Zhiqiang, could you maybe update the commit message with these two bits
> of information (the real issue explained by Eric, and the different
> Fixes: tag), and post v3?
> 
> This would be an actual fix and not a clean-up, so it doesn't need to
> wait for net-next to re-open.

Thanks for your reply.
I have updated the commit message as suggested by Eric. Even though I have read
Documentation/networking/netdev-FAQ.rst as you mentioned. I am now still a little
confused about the subject-prefix of v3 (net or net-next). And David Miller saied
the net-next tree is CLOSED.
Could you help me check whether the following v3 patch is ok?


Subject: [PATCH net v3] vxlan: remove the redundant gro_cells_destroy() calling.
OR
Subject: [PATCH net-next v3] vxlan: remove the redundant gro_cells_destroy() calling.

Commit ad6c9986bcb62 ("vxlan: Fix GRO cells race condition between
receive and link delete") fixed a race condition for the typical case a vxlan
device is dismantled from the current netns. But if a netns is dismantled,
vxlan_destroy_tunnels() is called to schedule a unregister_netdevice_queue()
of all the vxlan tunnels that are related to this netns.

In vxlan_destroy_tunnels(), gro_cells_destroy() is called and finished before
unregister_netdevice_queue(). This means that the gro_cells_destroy() call is
done too soon, for the same reasons explained in above commit.

So we need to fully respect the RCU rules, and thus must remove the
gro_cells_destroy() call or risk use after-free.

Fixes: 58ce31cca1ff ("vxlan: GRO support at tunnel layer")
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
V1->V2:
	- update the commit message suggeted by Eric Dumazet
	- update Fixes: tag

 drivers/net/vxlan.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 077f1b9f2761..d76dfed8d9bb 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4335,10 +4335,8 @@ static void vxlan_destroy_tunnels(struct net *net, struct list_head *head)
 		/* If vxlan->dev is in the same netns, it has already been added
 		 * to the list by the previous loop.
 		 */
-		if (!net_eq(dev_net(vxlan->dev), net)) {
-			gro_cells_destroy(&vxlan->gro_cells);
+		if (!net_eq(dev_net(vxlan->dev), net))
 			unregister_netdevice_queue(vxlan->dev, head);
-		}
 	}

 	for (h = 0; h < PORT_HASH_SIZE; ++h)
-- 
1.7.12.4

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Stefano Brivio]

On Sat, 16 Mar 2019 13:24:39 +0800
Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:

> I have updated the commit message as suggested by Eric. Even though I have read
> Documentation/networking/netdev-FAQ.rst as you mentioned. I am now still a little
> confused about the subject-prefix of v3 (net or net-next).

It's "net": this is a (likely critical) fix.

> And David Miller saied the net-next tree is CLOSED.

Right, but this is not for net-next anymore, given what Eric found.

> Could you help me check whether the following v3 patch is ok?
> 
> 
> Subject: [PATCH net v3] vxlan: remove the redundant gro_cells_destroy() calling.

This one. And it's not just redundant, so maybe something like:

	"[PATCH net v3] vxlan: Don't call gro_cells_destroy() before
	device is unregistered"

> OR
> Subject: [PATCH net-next v3] vxlan: remove the redundant gro_cells_destroy() calling.
> 
> Commit ad6c9986bcb62 ("vxlan: Fix GRO cells race condition between
> receive and link delete") fixed a race condition for the typical case a vxlan
> device is dismantled from the current netns. But if a netns is dismantled,
> vxlan_destroy_tunnels() is called to schedule a unregister_netdevice_queue()
> of all the vxlan tunnels that are related to this netns.
> 
> In vxlan_destroy_tunnels(), gro_cells_destroy() is called and finished before
> unregister_netdevice_queue(). This means that the gro_cells_destroy() call is
> done too soon, for the same reasons explained in above commit.
> 
> So we need to fully respect the RCU rules, and thus must remove the
> gro_cells_destroy() call or risk use after-free.
> 
> Fixes: 58ce31cca1ff ("vxlan: GRO support at tunnel layer")
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> V1->V2:

This will actually be v3.

> 	- update the commit message suggeted by Eric Dumazet
> 	- update Fixes: tag
> 
>  drivers/net/vxlan.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 077f1b9f2761..d76dfed8d9bb 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -4335,10 +4335,8 @@ static void vxlan_destroy_tunnels(struct net *net, struct list_head *head)
>  		/* If vxlan->dev is in the same netns, it has already been added
>  		 * to the list by the previous loop.
>  		 */
> -		if (!net_eq(dev_net(vxlan->dev), net)) {
> -			gro_cells_destroy(&vxlan->gro_cells);
> +		if (!net_eq(dev_net(vxlan->dev), net))
>  			unregister_netdevice_queue(vxlan->dev, head);
> -		}
>  	}
> 
>  	for (h = 0; h < PORT_HASH_SIZE; ++h)

Looks good to me, you can keep my Reviewed-by: tag for v3. Thanks!

-- 
Stefano

Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling.

[Zhiqiang Liu]

> On Sat, 16 Mar 2019 13:24:39 +0800
> Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> 
>> I have updated the commit message as suggested by Eric. Even though I have read
>> Documentation/networking/netdev-FAQ.rst as you mentioned. I am now still a little
>> confused about the subject-prefix of v3 (net or net-next).
> 
> It's "net": this is a (likely critical) fix.
> 
>> And David Miller saied the net-next tree is CLOSED.
> 
> Right, but this is not for net-next anymore, given what Eric found.
> 
>> Could you help me check whether the following v3 patch is ok?
>>
>>
>> Subject: [PATCH net v3] vxlan: remove the redundant gro_cells_destroy() calling.
> 
> This one. And it's not just redundant, so maybe something like:
> 
> 	"[PATCH net v3] vxlan: Don't call gro_cells_destroy() before
> 	device is unregistered"
> 
>> OR
>> Subject: [PATCH net-next v3] vxlan: remove the redundant gro_cells_destroy() calling.
>>
>> Commit ad6c9986bcb62 ("vxlan: Fix GRO cells race condition between
>> receive and link delete") fixed a race condition for the typical case a vxlan
>> device is dismantled from the current netns. But if a netns is dismantled,
>> vxlan_destroy_tunnels() is called to schedule a unregister_netdevice_queue()
>> of all the vxlan tunnels that are related to this netns.
>>
>> In vxlan_destroy_tunnels(), gro_cells_destroy() is called and finished before
>> unregister_netdevice_queue(). This means that the gro_cells_destroy() call is
>> done too soon, for the same reasons explained in above commit.
>>
>> So we need to fully respect the RCU rules, and thus must remove the
>> gro_cells_destroy() call or risk use after-free.
>>
>> Fixes: 58ce31cca1ff ("vxlan: GRO support at tunnel layer")
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>> V1->V2:
> 
> This will actually be v3.
> 
>> 	- update the commit message suggeted by Eric Dumazet
>> 	- update Fixes: tag
>>
>>  drivers/net/vxlan.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 077f1b9f2761..d76dfed8d9bb 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -4335,10 +4335,8 @@ static void vxlan_destroy_tunnels(struct net *net, struct list_head *head)
>>  		/* If vxlan->dev is in the same netns, it has already been added
>>  		 * to the list by the previous loop.
>>  		 */
>> -		if (!net_eq(dev_net(vxlan->dev), net)) {
>> -			gro_cells_destroy(&vxlan->gro_cells);
>> +		if (!net_eq(dev_net(vxlan->dev), net))
>>  			unregister_netdevice_queue(vxlan->dev, head);
>> -		}
>>  	}
>>
>>  	for (h = 0; h < PORT_HASH_SIZE; ++h)
> 
> Looks good to me, you can keep my Reviewed-by: tag for v3. Thanks!
> 

Thank you for your patience to reply. And thanks for Eric and David Miller again.
I will make the v3 as your suggestion.

[PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered

[Zhiqiang Liu]

Commit ad6c9986bcb62 ("vxlan: Fix GRO cells race condition between
receive and link delete") fixed a race condition for the typical case a vxlan
device is dismantled from the current netns. But if a netns is dismantled,
vxlan_destroy_tunnels() is called to schedule a unregister_netdevice_queue()
of all the vxlan tunnels that are related to this netns.

In vxlan_destroy_tunnels(), gro_cells_destroy() is called and finished before
unregister_netdevice_queue(). This means that the gro_cells_destroy() call is
done too soon, for the same reasons explained in above commit.

So we need to fully respect the RCU rules, and thus must remove the
gro_cells_destroy() call or risk use after-free.

Fixes: 58ce31cca1ff ("vxlan: GRO support at tunnel layer")
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
V1->V3:
	- update the commit message suggeted by Eric Dumazet
	- update Fixes: tag

 drivers/net/vxlan.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 077f1b9f2761..d76dfed8d9bb 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4335,10 +4335,8 @@ static void vxlan_destroy_tunnels(struct net *net, struct list_head *head)
 		/* If vxlan->dev is in the same netns, it has already been added
 		 * to the list by the previous loop.
 		 */
-		if (!net_eq(dev_net(vxlan->dev), net)) {
-			gro_cells_destroy(&vxlan->gro_cells);
+		if (!net_eq(dev_net(vxlan->dev), net))
 			unregister_netdevice_queue(vxlan->dev, head);
-		}
 	}

 	for (h = 0; h < PORT_HASH_SIZE; ++h)
-- 
1.7.12.4

Re: [PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered

[David Miller]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Date: Sat, 16 Mar 2019 17:02:54 +0800

> Commit ad6c9986bcb62 ("vxlan: Fix GRO cells race condition between
> receive and link delete") fixed a race condition for the typical case a vxlan
> device is dismantled from the current netns. But if a netns is dismantled,
> vxlan_destroy_tunnels() is called to schedule a unregister_netdevice_queue()
> of all the vxlan tunnels that are related to this netns.
> 
> In vxlan_destroy_tunnels(), gro_cells_destroy() is called and finished before
> unregister_netdevice_queue(). This means that the gro_cells_destroy() call is
> done too soon, for the same reasons explained in above commit.
> 
> So we need to fully respect the RCU rules, and thus must remove the
> gro_cells_destroy() call or risk use after-free.
> 
> Fixes: 58ce31cca1ff ("vxlan: GRO support at tunnel layer")
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> V1->V3:
> 	- update the commit message suggeted by Eric Dumazet
> 	- update Fixes: tag

Applied and queued up for -stable, thanks.