Re:Re: Re:Re: Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in del_chan
At 2017-08-10 05:00:19, "Cong Wang" wrote: >On Wed, Aug 9, 2017 at 12:17 AM, Gao Feng wrote: >> Hi Cong, >> >> Actually I have one question about the SOCK_RCU_FREE. >> I don't think it could resolve the issue you raised even though it exists >> really. >> >> I checked the SOCK_RCU_FREE, it just defer the __sk_destruct after one rcu >> period. >> But when it performs, someone still could find this sock by callid during >> the del_chan period and it may still deference the sock which may freed soon. >> >> The right flow should be following. >> del_chan() >> wait a rcu period >> sock_put() It is safe that someone gets the sock because it >> already hold sock refcnt. >> >> When using SOCK_RCU_FREE, its flow would be following. >> wait a rcu period >> del_chan() >> free the sock directly no sock refcnt check again. >> Because the del_chan happens after rcu wait, not before, so it isn't helpful >> with SOCK_RCU_FREE. > > >Yes, good point! With SOCK_RCU_FREE the sock_hold() should >not be needed. For RCU, unpublish should indeed happen before >grace period. Sorry, I couldn't understand why sock_hold() isn't necessary with SOCK_RCU_FREE. When lookup_chan finds the sock, it would return and reference it later. If no refcnt, how to protect the sock ? Best Regards Feng > > >> >> I don't know if I misunderstand the SOCK_RCU_FREE usage. >> >> But it is a good news that the del_chan is only invoked in pptp_release >> actually and it would wait a rcu period before sock_put. >> > >Looking at the code again, the reader lookup_chan() is actually >invoked in BH context, but neither add_chan() nor del_chan() >actually disables BH...
Re: Re:Re: Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in del_chan
On Wed, Aug 9, 2017 at 12:17 AM, Gao Feng wrote: > Hi Cong, > > Actually I have one question about the SOCK_RCU_FREE. > I don't think it could resolve the issue you raised even though it exists > really. > > I checked the SOCK_RCU_FREE, it just defer the __sk_destruct after one rcu > period. > But when it performs, someone still could find this sock by callid during the > del_chan period and it may still deference the sock which may freed soon. > > The right flow should be following. > del_chan() > wait a rcu period > sock_put() It is safe that someone gets the sock because it > already hold sock refcnt. > > When using SOCK_RCU_FREE, its flow would be following. > wait a rcu period > del_chan() > free the sock directly no sock refcnt check again. > Because the del_chan happens after rcu wait, not before, so it isn't helpful > with SOCK_RCU_FREE. Yes, good point! With SOCK_RCU_FREE the sock_hold() should not be needed. For RCU, unpublish should indeed happen before grace period. > > I don't know if I misunderstand the SOCK_RCU_FREE usage. > > But it is a good news that the del_chan is only invoked in pptp_release > actually and it would wait a rcu period before sock_put. > Looking at the code again, the reader lookup_chan() is actually invoked in BH context, but neither add_chan() nor del_chan() actually disables BH...
Re:Re: Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in del_chan
At 2017-08-09 03:45:53, "Cong Wang" wrote: >On Mon, Aug 7, 2017 at 6:10 PM, Gao Feng wrote: >> >> Sorry, I don't get you clearly. Why the sock_hold() isn't helpful? > >I already told you, the dereference happends before sock_hold(). > >sock = rcu_dereference(callid_sock[call_id]); >if (sock) { >opt = &sock->proto.pptp; >if (opt->dst_addr.sin_addr.s_addr != s_addr) <=== HERE >sock = NULL; >else >sock_hold(sk_pppox(sock)); >} > >If we don't wait for readers properly, sock could be freed at the >same time when deference it. Maybe I didn't show my explanation clearly. I think it won't happen as I mentioned in the last email. Because the pptp_release invokes the synchronize_rcu to make sure it, and actually there is no one which would invoke del_chan except pptp_release. It is guaranteed by that the pptp_release doesn't put the sock refcnt until complete all cleanup include marking sk_state as PPPOX_DEAD. In other words, even though the pptp_release is not the last user of this sock, the other one wouldn't invoke del_chan in pptp_sock_destruct. Because the condition "!(sk->sk_state & PPPOX_DEAD)" must be false. As summary, the del_chan and pppox_unbind_sock in pptp_sock_destruct are unnecessary. And it even brings confusing. Best Regards Feng > >> 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. > > >If this were true, then this code in pptp_sock_destruct() >would be unneeded: > >if (!(sk->sk_state & PPPOX_DEAD)) { >del_chan(pppox_sk(sk)); >pppox_unbind_sock(sk); >} > > >> >> >> 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. >> > > >I don't know. Looks like sock_orphan() is only called >in pptp_release(), but I am not sure if there is a case >we call sock destructor before release. > >Also note, this socket is very special, it doesn't support >poll(), sendmsg() or recvmsg()..