* [PATCH] vxlan: remove the redundant gro_cells_destroy() calling. @ 2019-03-15 10:06 Zhiqiang Liu 2019-03-15 11:54 ` Stefano Brivio ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-15 10:06 UTC (permalink / raw) To: davem, petrm, idosch, sd, sbrivio, mousuanming Cc: netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 10:06 [PATCH] vxlan: remove the redundant gro_cells_destroy() calling Zhiqiang Liu @ 2019-03-15 11:54 ` Stefano Brivio 2019-03-15 14:55 ` Zhiqiang Liu 2019-03-15 14:58 ` Eric Dumazet 2019-03-15 15:18 ` [PATCH v2] " Zhiqiang Liu 2 siblings, 1 reply; 20+ messages in thread From: Stefano Brivio @ 2019-03-15 11:54 UTC (permalink / raw) To: Zhiqiang Liu Cc: davem, petrm, idosch, sd, mousuanming, netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F) 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 11:54 ` Stefano Brivio @ 2019-03-15 14:55 ` Zhiqiang Liu 2019-03-15 15:25 ` Stefano Brivio 0 siblings, 1 reply; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-15 14:55 UTC (permalink / raw) To: Stefano Brivio Cc: davem, petrm, idosch, sd, mousuanming, netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F) > 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> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 14:55 ` Zhiqiang Liu @ 2019-03-15 15:25 ` Stefano Brivio 0 siblings, 0 replies; 20+ messages in thread From: Stefano Brivio @ 2019-03-15 15:25 UTC (permalink / raw) To: Zhiqiang Liu Cc: davem, petrm, idosch, sd, mousuanming, netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F), Eric Dumazet 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 10:06 [PATCH] vxlan: remove the redundant gro_cells_destroy() calling Zhiqiang Liu 2019-03-15 11:54 ` Stefano Brivio @ 2019-03-15 14:58 ` Eric Dumazet 2019-03-15 15:18 ` [PATCH v2] " Zhiqiang Liu 2 siblings, 0 replies; 20+ messages in thread From: Eric Dumazet @ 2019-03-15 14:58 UTC (permalink / raw) To: Zhiqiang Liu, davem, petrm, idosch, sd, sbrivio, mousuanming Cc: netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F) 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 10:06 [PATCH] vxlan: remove the redundant gro_cells_destroy() calling Zhiqiang Liu 2019-03-15 11:54 ` Stefano Brivio 2019-03-15 14:58 ` Eric Dumazet @ 2019-03-15 15:18 ` Zhiqiang Liu 2019-03-15 15:28 ` Stefano Brivio 2 siblings, 1 reply; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-15 15:18 UTC (permalink / raw) To: davem, petrm, idosch, sd, sbrivio, mousuanming Cc: netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F) 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 15:18 ` [PATCH v2] " Zhiqiang Liu @ 2019-03-15 15:28 ` Stefano Brivio 2019-03-15 16:06 ` Eric Dumazet 2019-03-15 18:02 ` [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling David Miller 0 siblings, 2 replies; 20+ messages in thread From: Stefano Brivio @ 2019-03-15 15:28 UTC (permalink / raw) To: Zhiqiang Liu Cc: davem, petrm, idosch, sd, mousuanming, netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F), Eric Dumazet 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 15:28 ` Stefano Brivio @ 2019-03-15 16:06 ` Eric Dumazet 2019-03-15 18:02 ` David Miller 2019-03-15 18:02 ` [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling David Miller 1 sibling, 1 reply; 20+ messages in thread From: Eric Dumazet @ 2019-03-15 16:06 UTC (permalink / raw) To: Stefano Brivio, Zhiqiang Liu Cc: davem, petrm, idosch, sd, mousuanming, netdev, Mingfangsen, Zhoukang (A), wangxiaogang (F), 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 16:06 ` Eric Dumazet @ 2019-03-15 18:02 ` David Miller 2019-03-15 18:56 ` Eric Dumazet 0 siblings, 1 reply; 20+ messages in thread From: David Miller @ 2019-03-15 18:02 UTC (permalink / raw) To: eric.dumazet Cc: sbrivio, liuzhiqiang26, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 18:02 ` David Miller @ 2019-03-15 18:56 ` Eric Dumazet 2019-03-15 21:08 ` Stefano Brivio 2019-03-16 2:33 ` Zhiqiang Liu 0 siblings, 2 replies; 20+ messages in thread From: Eric Dumazet @ 2019-03-15 18:56 UTC (permalink / raw) To: David Miller, eric.dumazet Cc: sbrivio, liuzhiqiang26, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 18:56 ` Eric Dumazet @ 2019-03-15 21:08 ` Stefano Brivio 2019-03-15 21:26 ` Eric Dumazet 2019-03-16 2:33 ` Zhiqiang Liu 1 sibling, 1 reply; 20+ messages in thread From: Stefano Brivio @ 2019-03-15 21:08 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, liuzhiqiang26, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 21:08 ` Stefano Brivio @ 2019-03-15 21:26 ` Eric Dumazet 2019-03-15 22:04 ` Stefano Brivio 0 siblings, 1 reply; 20+ messages in thread From: Eric Dumazet @ 2019-03-15 21:26 UTC (permalink / raw) To: Stefano Brivio, Eric Dumazet Cc: David Miller, liuzhiqiang26, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 21:26 ` Eric Dumazet @ 2019-03-15 22:04 ` Stefano Brivio 2019-03-16 5:24 ` Zhiqiang Liu 0 siblings, 1 reply; 20+ messages in thread From: Stefano Brivio @ 2019-03-15 22:04 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, liuzhiqiang26, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 22:04 ` Stefano Brivio @ 2019-03-16 5:24 ` Zhiqiang Liu 2019-03-16 5:45 ` Stefano Brivio 0 siblings, 1 reply; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-16 5:24 UTC (permalink / raw) To: Stefano Brivio, Eric Dumazet Cc: David Miller, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 : > 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-16 5:24 ` Zhiqiang Liu @ 2019-03-16 5:45 ` Stefano Brivio 2019-03-16 6:27 ` Zhiqiang Liu 0 siblings, 1 reply; 20+ messages in thread From: Stefano Brivio @ 2019-03-16 5:45 UTC (permalink / raw) To: Zhiqiang Liu Cc: Eric Dumazet, David Miller, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-16 5:45 ` Stefano Brivio @ 2019-03-16 6:27 ` Zhiqiang Liu 0 siblings, 0 replies; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-16 6:27 UTC (permalink / raw) To: Stefano Brivio Cc: Eric Dumazet, David Miller, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 > 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 18:56 ` Eric Dumazet 2019-03-15 21:08 ` Stefano Brivio @ 2019-03-16 2:33 ` Zhiqiang Liu 2019-03-16 9:02 ` [PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered Zhiqiang Liu 1 sibling, 1 reply; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-16 2:33 UTC (permalink / raw) To: Eric Dumazet, David Miller Cc: sbrivio, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 >>> 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered 2019-03-16 2:33 ` Zhiqiang Liu @ 2019-03-16 9:02 ` Zhiqiang Liu 2019-03-19 0:08 ` David Miller 0 siblings, 1 reply; 20+ messages in thread From: Zhiqiang Liu @ 2019-03-16 9:02 UTC (permalink / raw) To: Eric Dumazet, David Miller, sbrivio Cc: petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered 2019-03-16 9:02 ` [PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered Zhiqiang Liu @ 2019-03-19 0:08 ` David Miller 0 siblings, 0 replies; 20+ messages in thread From: David Miller @ 2019-03-19 0:08 UTC (permalink / raw) To: liuzhiqiang26 Cc: eric.dumazet, sbrivio, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling. 2019-03-15 15:28 ` Stefano Brivio 2019-03-15 16:06 ` Eric Dumazet @ 2019-03-15 18:02 ` David Miller 1 sibling, 0 replies; 20+ messages in thread From: David Miller @ 2019-03-15 18:02 UTC (permalink / raw) To: sbrivio Cc: liuzhiqiang26, petrm, idosch, sd, mousuanming, netdev, mingfangsen, zhoukang7, wangxiaogang3, eric.dumazet 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-03-19 0:08 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-15 10:06 [PATCH] vxlan: remove the redundant gro_cells_destroy() calling Zhiqiang Liu 2019-03-15 11:54 ` Stefano Brivio 2019-03-15 14:55 ` Zhiqiang Liu 2019-03-15 15:25 ` Stefano Brivio 2019-03-15 14:58 ` Eric Dumazet 2019-03-15 15:18 ` [PATCH v2] " Zhiqiang Liu 2019-03-15 15:28 ` Stefano Brivio 2019-03-15 16:06 ` Eric Dumazet 2019-03-15 18:02 ` David Miller 2019-03-15 18:56 ` Eric Dumazet 2019-03-15 21:08 ` Stefano Brivio 2019-03-15 21:26 ` Eric Dumazet 2019-03-15 22:04 ` Stefano Brivio 2019-03-16 5:24 ` Zhiqiang Liu 2019-03-16 5:45 ` Stefano Brivio 2019-03-16 6:27 ` Zhiqiang Liu 2019-03-16 2:33 ` Zhiqiang Liu 2019-03-16 9:02 ` [PATCH net v3] vxlan: Don't call gro_cells_destroy() before device is unregistered Zhiqiang Liu 2019-03-19 0:08 ` David Miller 2019-03-15 18:02 ` [PATCH v2] vxlan: remove the redundant gro_cells_destroy() calling David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.