On Wed, Aug 12, 2020 at 2:28 AM Johan Knöös <jkn...@google.com> wrote:
> On Mon, Aug 10, 2020 at 10:59 PM Tonghao Zhang <xiangxia.m....@gmail.com> > wrote: > > > > On Tue, Aug 11, 2020 at 12:08 PM Cong Wang <xiyou.wangc...@gmail.com> > 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 <xiyou.wangc...@gmail.com> > 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(&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? Yes > > -- Best regards, Tonghao
_______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
