Re: [Devel] Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
"Denis V. Lunev" <[EMAIL PROTECTED]> writes: > The presence of the message in the queue during rtnl_unlock is quite > possible as normal user->kernel message processing path for rtnl is the > following: > > netlink_sendmsg >netlink_unicast > netlink_sendskb > skb_queue_tail > netlink_data_ready > rtnetlink_rcv > mutex_lock(&rtnl_mutex); > netlink_run_queue(sk, qlen, &rtnetlink_rcv_msg); > mutex_unlock(&rtnl_mutex); > > so, the presence of the packet in the rtnl queue on rtnl_unlock is > normal race with a rtnetlink_rcv for me. Yes. That is what I saw in practice as well. Thanks for confirming this. It happened to reproducible because I had a dhcp client asking for a list of links in parallel with the actual link coming up during boot. Looking at netlink_unicast and netlink_broadcast I am generally convinced that we can remove the call of sk_data_ready in rtnl_unlock. I think those are the only two possible paths through there and I don't see how we could miss a processing a packet on the way through there. What would be nice is if we could figure out how to eliminate this race. As that would allow netlink packets to be processed synchronously and we could actually use current for security checks, and for getting the context of the calling process. Right now we are 99% of the way there but because of the above race the code must all be written as if netlink packets were coming in completely asynchronously. Which is unfortunate and a pain. Eric - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Devel] Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
Patrick McHardy wrote: > Eric W. Biederman wrote: >> Patrick McHardy <[EMAIL PROTECTED]> writes: >> >> >>> Maybe I can save you some time: we used to do down_trylock() >>> for the rtnl mutex, so senders would simply return if someone >>> else was already processing the queue *or* the rtnl was locked >>> for some other reason. In the first case the process already >>> processing the queue would also process the new messages, but >>> if it the rtnl was locked for some other reason (for example >>> during module registration) the message would sit in the >>> queue until the next rtnetlink sendmsg call, which is why >>> rtnl_unlock does queue processing. Commit 6756ae4b changed >>> the down_trylock to mutex_lock, so senders will now simply wait >>> until the mutex is released and then call netlink_run_queue >>> themselves. This means its not needed anymore. >> >> Sounds reasonable. >> >> I started looking through the code paths and I currently cannot >> see anything that would leave a message on a kernel rtnl socket. >> >> However I did a quick test adding a WARN_ON if there were any messages >> found in the queue during rtnl_unlock and I found this code path >> getting invoked from linkwatch_event. So there is clearly something I >> don't understand, and it sounds at odds just a bit from your >> description. > > > That sounds like a bug. Did you place the WARN_ON before or after > the mutex_unlock()? The presence of the message in the queue during rtnl_unlock is quite possible as normal user->kernel message processing path for rtnl is the following: netlink_sendmsg netlink_unicast netlink_sendskb skb_queue_tail netlink_data_ready rtnetlink_rcv mutex_lock(&rtnl_mutex); netlink_run_queue(sk, qlen, &rtnetlink_rcv_msg); mutex_unlock(&rtnl_mutex); so, the presence of the packet in the rtnl queue on rtnl_unlock is normal race with a rtnetlink_rcv for me. Regards, Den - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Devel] Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
Hmm, so it looks like we do not need this queue processing at all... Regards, Den Eric W. Biederman wrote: > Patrick McHardy <[EMAIL PROTECTED]> writes: > >> Maybe I can save you some time: we used to do down_trylock() >> for the rtnl mutex, so senders would simply return if someone >> else was already processing the queue *or* the rtnl was locked >> for some other reason. In the first case the process already >> processing the queue would also process the new messages, but >> if it the rtnl was locked for some other reason (for example >> during module registration) the message would sit in the >> queue until the next rtnetlink sendmsg call, which is why >> rtnl_unlock does queue processing. Commit 6756ae4b changed >> the down_trylock to mutex_lock, so senders will now simply wait >> until the mutex is released and then call netlink_run_queue >> themselves. This means its not needed anymore. > > Sounds reasonable. > > I started looking through the code paths and I currently cannot > see anything that would leave a message on a kernel rtnl socket. > > However I did a quick test adding a WARN_ON if there were any messages > found in the queue during rtnl_unlock and I found this code path > getting invoked from linkwatch_event. So there is clearly something I > don't understand, and it sounds at odds just a bit from your > description. > > If we can remove the extra queue processing that would be great, > as it looks like a nice way to simplify the locking and the special > cases in the code. > > Eric - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
