On Mon, Aug 10, 2020 at 10:59 PM Tonghao Zhang <xiangxia.m.yue@xxxxxxxxx> wrote: > > On Tue, Aug 11, 2020 at 12:08 PM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote: > > > > On Mon, Aug 10, 2020 at 8:27 PM Tonghao Zhang <xiangxia.m.yue@xxxxxxxxx> wrote: > > > > > > On Tue, Aug 11, 2020 at 10:24 AM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote: > > > > > > > > On Mon, Aug 10, 2020 at 6:16 PM Tonghao Zhang <xiangxia.m.yue@xxxxxxxxx> 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.yue@xxxxxxxxx/ > > > > > > > > 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(&old->rcu, mask_array_rcu_cb); > > ovs_unlock() > > ... > > ovs_flow_tbl_destroy() > > => call_rcu(&old->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(&ma->rcu, mask_array_rcu_cb); > ->table_instance_destroy > ->tbl_mask_array_realloc(Shrink the mask array if necessary) > ->call_rcu(&old->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(&ma->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?
