Re: [ovs-discuss] Double free in recent kernels after memleak fix
On Wed, Aug 12, 2020 at 2:28 AM Johan Knöös wrote: > On Mon, Aug 10, 2020 at 10:59 PM Tonghao Zhang > wrote: > > > > On Tue, Aug 11, 2020 at 12:08 PM Cong Wang > wrote: > > > > > > On Mon, Aug 10, 2020 at 8:27 PM Tonghao Zhang < > xiangxia.m@gmail.com> wrote: > > > > > > > > On Tue, Aug 11, 2020 at 10:24 AM Cong Wang > wrote: > > > > > > > > > > On Mon, Aug 10, 2020 at 6:16 PM Tonghao Zhang < > xiangxia.m@gmail.com> wrote: > > > > > > Hi all, I send a patch to fix this. The rcu warnings disappear. I > > > > > > don't reproduce the double free issue. > > > > > > But I guess this patch may address this issue. > > > > > > > > > > > > > http://patchwork.ozlabs.org/project/netdev/patch/20200811011001.75690-1-xiangxia.m@gmail.com/ > > > > > > > > > > I don't see how your patch address the double-free, as we still > > > > > free mask array twice after your patch: once in > tbl_mask_array_realloc() > > > > > and once in ovs_flow_tbl_destroy(). > > > > Hi Cong. > > > > Before my patch, we use the ovsl_dereference > > > > (rcu_dereference_protected) in the rcu callback. > > > > ovs_flow_tbl_destroy > > > > ->table_instance_destroy > > > > ->table_instance_flow_free > > > > ->flow_mask_remove > > > > ASSERT_OVSL(will print warning) > > > > ->tbl_mask_array_del_mask > > > > ovsl_dereference(rcu usage warning) > > > > > > > > > > I understand how your patch addresses the RCU annotation issue, > > > which is different from double-free. > > > > > > > > > > so we should invoke the table_instance_destroy or others under > > > > ovs_lock to avoid (ASSERT_OVSL and rcu usage warning). > > > > > > Of course... I never doubt it. > > > > > > > > > > with this patch, we reallocate the mask_array under ovs_lock, and > free > > > > it in the rcu callback. Without it, we reallocate and free it in the > > > > rcu callback. > > > > I think we may fix it with this patch. > > > > > > Does it matter which context tbl_mask_array_realloc() is called? > > > Even with ovs_lock, we can still double free: > > > > > > ovs_lock() > > > tbl_mask_array_realloc() > > > => call_rcu(>rcu, mask_array_rcu_cb); > > > ovs_unlock() > > > ... > > > ovs_flow_tbl_destroy() > > > => call_rcu(>rcu, mask_array_rcu_cb); > > > > > > So still twice, right? To fix the double-free, we have to eliminate one > > > of them, don't we? ;) > > No > > Without my patch: in rcu callback: > > ovs_flow_tbl_destroy > > ->call_rcu(>rcu, mask_array_rcu_cb); > > ->table_instance_destroy > > ->tbl_mask_array_realloc(Shrink the mask array if necessary) > > ->call_rcu(>rcu, mask_array_rcu_cb); > > > > With the patch: > > ovs_lock > > table_instance_flow_flush (free the flow) > > tbl_mask_array_realloc(shrink the mask array if necessary, will free > > mask_array in rcu(mask_array_rcu_cb) and rcu_assign_pointer new > > mask_array) > > ovs_unlock > > > > in rcu callback: > > ovs_flow_tbl_destroy > > call_rcu(>rcu, mask_array_rcu_cb);(that is new mask_array) > > > > > > > > > > > > > > Have you tried my patch which is supposed to address this > double-free? > > > > I don't reproduce it. but your patch does not avoid ruc usage warning > > > > and ASSERT_OVSL. > > > > > > Sure, I never intend to fix anything else but double-free. The > $subject is > > > about double free, I double checked. ;) > > > > > > Thanks. > > > > > > > > -- > > Best regards, Tonghao > > Cong and Tonghao, thanks for your patches. I cannot repro the double > free with either of them, and the "suspicious RCU usage" and the > ASSERT_OVSL warnings are also gone with Tonghao's patch. > > Tonghao, from your sequence above it looks like it should fix the > https://elixir.bootlin.com/linux/v5.5.17/source/kernel/rcu/tree.c#L2239 > warning, correct? Yes > > -- Best regards, Tonghao ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] Double free in recent kernels after memleak fix
On Mon, Aug 10, 2020 at 10:59 PM Tonghao Zhang wrote: > > On Tue, Aug 11, 2020 at 12:08 PM Cong Wang wrote: > > > > On Mon, Aug 10, 2020 at 8:27 PM Tonghao Zhang > > wrote: > > > > > > On Tue, Aug 11, 2020 at 10:24 AM Cong Wang > > > wrote: > > > > > > > > On Mon, Aug 10, 2020 at 6:16 PM Tonghao Zhang > > > > wrote: > > > > > Hi all, I send a patch to fix this. The rcu warnings disappear. I > > > > > don't reproduce the double free issue. > > > > > But I guess this patch may address this issue. > > > > > > > > > > http://patchwork.ozlabs.org/project/netdev/patch/20200811011001.75690-1-xiangxia.m@gmail.com/ > > > > > > > > I don't see how your patch address the double-free, as we still > > > > free mask array twice after your patch: once in tbl_mask_array_realloc() > > > > and once in ovs_flow_tbl_destroy(). > > > Hi Cong. > > > Before my patch, we use the ovsl_dereference > > > (rcu_dereference_protected) in the rcu callback. > > > ovs_flow_tbl_destroy > > > ->table_instance_destroy > > > ->table_instance_flow_free > > > ->flow_mask_remove > > > ASSERT_OVSL(will print warning) > > > ->tbl_mask_array_del_mask > > > ovsl_dereference(rcu usage warning) > > > > > > > I understand how your patch addresses the RCU annotation issue, > > which is different from double-free. > > > > > > > so we should invoke the table_instance_destroy or others under > > > ovs_lock to avoid (ASSERT_OVSL and rcu usage warning). > > > > Of course... I never doubt it. > > > > > > > with this patch, we reallocate the mask_array under ovs_lock, and free > > > it in the rcu callback. Without it, we reallocate and free it in the > > > rcu callback. > > > I think we may fix it with this patch. > > > > Does it matter which context tbl_mask_array_realloc() is called? > > Even with ovs_lock, we can still double free: > > > > ovs_lock() > > tbl_mask_array_realloc() > > => call_rcu(>rcu, mask_array_rcu_cb); > > ovs_unlock() > > ... > > ovs_flow_tbl_destroy() > > => call_rcu(>rcu, mask_array_rcu_cb); > > > > So still twice, right? To fix the double-free, we have to eliminate one > > of them, don't we? ;) > No > Without my patch: in rcu callback: > ovs_flow_tbl_destroy > ->call_rcu(>rcu, mask_array_rcu_cb); > ->table_instance_destroy > ->tbl_mask_array_realloc(Shrink the mask array if necessary) > ->call_rcu(>rcu, mask_array_rcu_cb); > > With the patch: > ovs_lock > table_instance_flow_flush (free the flow) > tbl_mask_array_realloc(shrink the mask array if necessary, will free > mask_array in rcu(mask_array_rcu_cb) and rcu_assign_pointer new > mask_array) > ovs_unlock > > in rcu callback: > ovs_flow_tbl_destroy > call_rcu(>rcu, mask_array_rcu_cb);(that is new mask_array) Ah, I see, I thought the mask array was cached in caller and passed along, it is in fact refetched via >table. Thanks. ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] Double free in recent kernels after memleak fix
On Mon, Aug 10, 2020 at 10:59 PM Tonghao Zhang wrote: > > On Tue, Aug 11, 2020 at 12:08 PM Cong Wang wrote: > > > > On Mon, Aug 10, 2020 at 8:27 PM Tonghao Zhang > > wrote: > > > > > > On Tue, Aug 11, 2020 at 10:24 AM Cong Wang > > > wrote: > > > > > > > > On Mon, Aug 10, 2020 at 6:16 PM Tonghao Zhang > > > > wrote: > > > > > Hi all, I send a patch to fix this. The rcu warnings disappear. I > > > > > don't reproduce the double free issue. > > > > > But I guess this patch may address this issue. > > > > > > > > > > http://patchwork.ozlabs.org/project/netdev/patch/20200811011001.75690-1-xiangxia.m@gmail.com/ > > > > > > > > I don't see how your patch address the double-free, as we still > > > > free mask array twice after your patch: once in tbl_mask_array_realloc() > > > > and once in ovs_flow_tbl_destroy(). > > > Hi Cong. > > > Before my patch, we use the ovsl_dereference > > > (rcu_dereference_protected) in the rcu callback. > > > ovs_flow_tbl_destroy > > > ->table_instance_destroy > > > ->table_instance_flow_free > > > ->flow_mask_remove > > > ASSERT_OVSL(will print warning) > > > ->tbl_mask_array_del_mask > > > ovsl_dereference(rcu usage warning) > > > > > > > I understand how your patch addresses the RCU annotation issue, > > which is different from double-free. > > > > > > > so we should invoke the table_instance_destroy or others under > > > ovs_lock to avoid (ASSERT_OVSL and rcu usage warning). > > > > Of course... I never doubt it. > > > > > > > with this patch, we reallocate the mask_array under ovs_lock, and free > > > it in the rcu callback. Without it, we reallocate and free it in the > > > rcu callback. > > > I think we may fix it with this patch. > > > > Does it matter which context tbl_mask_array_realloc() is called? > > Even with ovs_lock, we can still double free: > > > > ovs_lock() > > tbl_mask_array_realloc() > > => call_rcu(>rcu, mask_array_rcu_cb); > > ovs_unlock() > > ... > > ovs_flow_tbl_destroy() > > => call_rcu(>rcu, mask_array_rcu_cb); > > > > So still twice, right? To fix the double-free, we have to eliminate one > > of them, don't we? ;) > No > Without my patch: in rcu callback: > ovs_flow_tbl_destroy > ->call_rcu(>rcu, mask_array_rcu_cb); > ->table_instance_destroy > ->tbl_mask_array_realloc(Shrink the mask array if necessary) > ->call_rcu(>rcu, mask_array_rcu_cb); > > With the patch: > ovs_lock > table_instance_flow_flush (free the flow) > tbl_mask_array_realloc(shrink the mask array if necessary, will free > mask_array in rcu(mask_array_rcu_cb) and rcu_assign_pointer new > mask_array) > ovs_unlock > > in rcu callback: > ovs_flow_tbl_destroy > call_rcu(>rcu, mask_array_rcu_cb);(that is new mask_array) > > > > > > > > > > Have you tried my patch which is supposed to address this double-free? > > > I don't reproduce it. but your patch does not avoid ruc usage warning > > > and ASSERT_OVSL. > > > > Sure, I never intend to fix anything else but double-free. The $subject is > > about double free, I double checked. ;) > > > > Thanks. > > > > -- > Best regards, Tonghao Cong and Tonghao, thanks for your patches. I cannot repro the double free with either of them, and the "suspicious RCU usage" and the ASSERT_OVSL warnings are also gone with Tonghao's patch. Tonghao, from your sequence above it looks like it should fix the https://elixir.bootlin.com/linux/v5.5.17/source/kernel/rcu/tree.c#L2239 warning, correct? ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
[ovs-discuss] [OVN] ToR as the gateway
Hi, In http://www.openvswitch.org/support/dist-docs/ovn-architecture.7.html, I see this. For connecting to gateways, in addition to Geneve and STT, OVN supports VXLAN, because only VXLAN support is common on top-of-rack (ToR) switches. Does anyone have experiences in using physical ToR as the gateway with VXLAN to connect private virtual networks to external? Thanks! Tony ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
[ovs-discuss] [OVN] Dependency to OVS
Hi, Is there any version dependency between OVN and OVS, like which OVS has to be used for certain OVN release? Is such info shared anywhere? Thanks! Tony ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
Re: [ovs-discuss] Double free in recent kernels after memleak fix
On Tue, Aug 11, 2020 at 12:08 PM Cong Wang wrote: > > On Mon, Aug 10, 2020 at 8:27 PM Tonghao Zhang > wrote: > > > > On Tue, Aug 11, 2020 at 10:24 AM Cong Wang wrote: > > > > > > On Mon, Aug 10, 2020 at 6:16 PM Tonghao Zhang > > > wrote: > > > > Hi all, I send a patch to fix this. The rcu warnings disappear. I > > > > don't reproduce the double free issue. > > > > But I guess this patch may address this issue. > > > > > > > > http://patchwork.ozlabs.org/project/netdev/patch/20200811011001.75690-1-xiangxia.m@gmail.com/ > > > > > > I don't see how your patch address the double-free, as we still > > > free mask array twice after your patch: once in tbl_mask_array_realloc() > > > and once in ovs_flow_tbl_destroy(). > > Hi Cong. > > Before my patch, we use the ovsl_dereference > > (rcu_dereference_protected) in the rcu callback. > > ovs_flow_tbl_destroy > > ->table_instance_destroy > > ->table_instance_flow_free > > ->flow_mask_remove > > ASSERT_OVSL(will print warning) > > ->tbl_mask_array_del_mask > > ovsl_dereference(rcu usage warning) > > > > I understand how your patch addresses the RCU annotation issue, > which is different from double-free. > > > > so we should invoke the table_instance_destroy or others under > > ovs_lock to avoid (ASSERT_OVSL and rcu usage warning). > > Of course... I never doubt it. > > > > with this patch, we reallocate the mask_array under ovs_lock, and free > > it in the rcu callback. Without it, we reallocate and free it in the > > rcu callback. > > I think we may fix it with this patch. > > Does it matter which context tbl_mask_array_realloc() is called? > Even with ovs_lock, we can still double free: > > ovs_lock() > tbl_mask_array_realloc() > => call_rcu(>rcu, mask_array_rcu_cb); > ovs_unlock() > ... > ovs_flow_tbl_destroy() > => call_rcu(>rcu, mask_array_rcu_cb); > > So still twice, right? To fix the double-free, we have to eliminate one > of them, don't we? ;) No Without my patch: in rcu callback: ovs_flow_tbl_destroy ->call_rcu(>rcu, mask_array_rcu_cb); ->table_instance_destroy ->tbl_mask_array_realloc(Shrink the mask array if necessary) ->call_rcu(>rcu, mask_array_rcu_cb); With the patch: ovs_lock table_instance_flow_flush (free the flow) tbl_mask_array_realloc(shrink the mask array if necessary, will free mask_array in rcu(mask_array_rcu_cb) and rcu_assign_pointer new mask_array) ovs_unlock in rcu callback: ovs_flow_tbl_destroy call_rcu(>rcu, mask_array_rcu_cb);(that is new mask_array) > > > > > > Have you tried my patch which is supposed to address this double-free? > > I don't reproduce it. but your patch does not avoid ruc usage warning > > and ASSERT_OVSL. > > Sure, I never intend to fix anything else but double-free. The $subject is > about double free, I double checked. ;) > > Thanks. -- Best regards, Tonghao ___ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
