Messages in this thread | | | From | Jon Maloy <> | Subject | RE: [PATCH] tipc: ensure skb->lock is initialised | Date | Thu, 11 Jul 2019 12:55:12 +0000 |
| |
> -----Original Message----- > From: Chris Packham <Chris.Packham@alliedtelesis.co.nz> > Sent: 10-Jul-19 16:58 > To: Jon Maloy <jon.maloy@ericsson.com>; Eric Dumazet > <eric.dumazet@gmail.com>; ying.xue@windriver.com; > davem@davemloft.net > Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] tipc: ensure skb->lock is initialised > > > On 11/07/19 1:10 AM, Jon Maloy wrote: > >> -----Original Message----- > >> From: Eric Dumazet <eric.dumazet@gmail.com> > >> Sent: 10-Jul-19 04:00 > >> To: Jon Maloy <jon.maloy@ericsson.com>; Eric Dumazet > >> <eric.dumazet@gmail.com>; Chris Packham > >> <Chris.Packham@alliedtelesis.co.nz>; ying.xue@windriver.com; > >> davem@davemloft.net > >> Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; > >> linux- kernel@vger.kernel.org > >> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised > >> > >> > >> > >> On 7/9/19 10:15 PM, Jon Maloy wrote: > >>> > >>> It is not only for lockdep purposes, -it is essential. But please > >>> provide details > >> about where you see that more fixes are needed. > >>> > >> > >> Simple fact that you detect a problem only when skb_queue_purge() is > >> called should talk by itself. > >> > >> As I stated, there are many places where the list is manipulated > >> _without_ its spinlock being held. > > > > Yes, and that is the way it should be on the send path. > > > >> > >> You want consistency, then > >> > >> - grab the spinlock all the time. > >> - Or do not ever use it. > > > > That is exactly what we are doing. > > - The send path doesn't need the spinlock, and never grabs it. > > - The receive path does need it, and always grabs it. > > > > However, since we don't know from the beginning which path a created > > message will follow, we initialize the queue spinlock "just in case" > > when it is created, even though it may never be used later. > > You can see this as a violation of the principle you are stating > > above, but it is a prize that is worth paying, given savings in code > > volume, complexity and performance. > > > >> > >> Do not initialize the spinlock just in case a path will use > >> skb_queue_purge() (instead of using __skb_queue_purge()) > > > > I am ok with that. I think we can agree that Chris goes for that > > solution, so we can get this bug fixed. > > So would you like a v2 with an improved commit message? I note that I said > skb->lock instead of head->lock in the subject line so at least that should be > corrected.
Yes, unless Eric has any more objections. I addition, I have realized that we can change from skb_queue_head_init() to __skb_queue_head_init() at all the disputed locations in the socket code. Then, we do a separate call to spin_lock_init(&list->lock) at the moment we find that the message will follow the receive path, i.e., in tipc_node_xmit(). That should make everybody happy. I will post a patch when net-next re-opens.
BR ///jon
| |