On Sat, Jun 10, 2017 at 1:01 AM, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> On Fri, Jun 9, 2017 at 8:56 AM, Eric Dumazet <eric.duma...@gmail.com> wrote: >> On Fri, 2017-06-09 at 14:24 +0800, Xin Long wrote: >>> On Fri, Jun 9, 2017 at 8:59 AM, Cong Wang <xiyou.wangc...@gmail.com> wrote: >>> >>> > On Thu, Jun 8, 2017 at 1:33 PM, Eric Dumazet <eric.duma...@gmail.com> >>> > wrote: >>> >> I mentioned (in https://lkml.org/lkml/2017/5/31/619 ) that we might need >>> >> to defer freeing after rcu grace period but for some reason decided it >>> >> was not needed. >>> Yes, this one could fix it. >>> >>> > >>> > This one makes sense, it is the second time I saw the use-after-free >>> > in igmp code, both are because we don't respect the RCU rule to free >>> > an element in the list. >>> > >>> >> >>> >> What about : >>> > >>> > But not sure if all ip_ma_put() callers want ip_mc_clear_src(). >>> If that's problem, there may be another way: >>> >>> leave ip_mc_clear_src as it is, just add pmc->lock to protect this call. >>> >>> this use-after-free was actually caused by using pmc->sources/tomb >>> in add_grec while ip_mc_clear_src is freeing them. add_grec is already >>> under pmc->lock, so to add pmc->lock for ip_mc_clear_src should be >>> enough to protect the list pmc->sources/tomb. >>> >>> wdyt ? >> >> This would we weird. >> >> When we free skb components, we do not grab a spinlock. >> >> When we free something, just make sure we must be the last user of it. >> >> RCU rules -> Must respect RCU grace period before delete. >> >> No need for extra spinlock. > > This is what I thought in my first response, until I realized > it is not pure RCU, otherwise pmc->lock should not be taken > in igmpv3_send_cr(). It seems the code is mixing the use > of spinlock and RCU. rcu lock is for pmc not being freed, and spinlock is for pmc's members' modification. is there some rule these two locks should be mixed? > > We need RCU anyway, ip_check_mc_rcu() is the real fast > path where we don't take spinlock. I think we will need more > work. It seems all add_grec() callings needs spinlock, maybe add_grec modifies pmc's member. it's hard to drop spinlock. from ip_check_mc_rcu you mentioned about, it should be right to call ip_mc_clear_src after rcu grace period, like Eric's patch.
