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
