On 11/07/19 1:10 AM, Jon Maloy wrote: >> -----Original Message----- >> From: Eric Dumazet <[email protected]> >> Sent: 10-Jul-19 04:00 >> To: Jon Maloy <[email protected]>; Eric Dumazet >> <[email protected]>; Chris Packham >> <[email protected]>; [email protected]; >> [email protected] >> Cc: [email protected]; [email protected]; linux- >> [email protected] >> 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.
