At 2017-08-08 01:17:02, "Cong Wang" <xiyou.wangc...@gmail.com> wrote: >On Sun, Aug 6, 2017 at 6:32 PM, Gao Feng <gfree.w...@vip.163.com> wrote: >> I think the RCU should be supposed to avoid the race between del_chan and >> lookup_chan. > >More precisely, it is callid_sock which is protected by RCU. > >Unless I miss any other code path, pptp_exit_module() is >problematic too, I don't think it can just vfree() the whole thing. > > >> The synchronize_rcu could make sure if there was one which calls lookup_chan >> in this period, it would be finished and the sock refcnt is increased if >> necessary. >> >> So I think it is ok to invoke sock_put directly without SOCK_RCU_FREE, >> because the lookup_chan caller has already hold the sock refcnt, >>
> Hi Cong, I just thought about this issue last night, then I get your this email this morning. >If you mean the sock_hold() inside lookup_chan(), no, >it doesn't help because we already dereference the sock >before it. > Sorry, I don't get you clearly. Why the sock_hold() isn't helpful? The pptp_release invokes synchronize_rcu after del_chan, it could make sure the others has increased the sock refcnt if necessary and the lookup is over. There is no one could get the sock after synchronize_rcu in pptp_release. But I think about another problem. It seems the pptp_sock_destruct should not invoke del_chan and pppox_unbind_sock. Because when the sock refcnt is 0, the pptp_release must have be invoked already. There are two cases totally. 1. when pptp_release invokes sock_put, the refcnt is 0. The del_chan and pppox_unbind_sock are invoked. 2. when pptp_release invokes sock_put, the refcnt is not 0. It means someone holds the sock during the period pptp_release invokes del_chan. Then someone invokes sock_put and the sock refcnt reach 0, it would invoke sk_free and invokes pptp_sock_destruct. So it is unnecessary to invoke del_chan and pppox_unbind_sock again. And it would bring a race issue even if the pptp_sock_destruct invoked del_chan. If so, I would send another patch for it. >Also, lookup_chan_dst() does not have a refcnt, I don't >find any code preventing it deref'ing other sock in callid_sock >than the calling one. Sorry, the last email is html format, not text. So I send it with text format again. Best Regards Feng
