Re: [PATCH net] nfc: avoid potential race condition

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Samuel Ortiz <sameo@linux.intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	"John W. Linville" <linville@tuxdriver.com>,
	netdev@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH net] nfc: avoid potential race condition
Date: Mon, 27 Sep 2021 17:49:47 +0300	[thread overview]
Message-ID: <20210927144947.GI2083@kadam> (raw)
In-Reply-To: <81b648d2-0e20-e5ac-e2ff-a1b8b8ea83a8@canonical.com>

On Mon, Sep 27, 2021 at 09:44:08AM +0200, Krzysztof Kozlowski wrote:
> On 24/09/2021 22:14, Jakub Kicinski wrote:
> > On Fri, 24 Sep 2021 10:21:33 +0200 Krzysztof Kozlowski wrote:
> >> On 23/09/2021 14:22, Dan Carpenter wrote:
> >>> On Thu, Sep 23, 2021 at 09:26:51AM +0200, Krzysztof Kozlowski wrote:  
> >>>> On 23/09/2021 08:50, Dan Carpenter wrote:  
> >>  [...]  
> >>>>
> >>>> I think the difference between this llcp_sock code and above transport,
> >>>> is lack of writer to llcp_sock->local with whom you could race.
> >>>>
> >>>> Commits c0cfa2d8a788fcf4 and 6a2c0962105ae8ce causing the
> >>>> multi-transport race show nicely assigns to vsk->transport when module
> >>>> is unloaded.
> >>>>
> >>>> Here however there is no writer to llcp_sock->local, except bind and
> >>>> connect and their error paths. The readers which you modify here, have
> >>>> to happen after bind/connect. You cannot have getsockopt() or release()
> >>>> before bind/connect, can you? Unless you mean here the bind error path,
> >>>> where someone calls getsockopt() in the middle of bind()? Is it even
> >>>> possible?
> >>>>  
> >>>
> >>> I don't know if this is a real issue either.
> >>>
> >>> Racing with bind would be harmless.  The local pointer would be NULL and
> >>> it would return harmlessly.  You would have to race with release and
> >>> have a third trying to release local devices.  (Again that might be
> >>> wild imagination.  It may not be possible).  
> >>
> >> Indeed. The code looks reasonable, though, so even if race is not really
> >> reproducible:
> >>
> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > 
> > Would you mind making a call if this is net (which will mean stable) or
> > net-next material (without the Fixes tags) and reposting? Thanks! :)
> 
> Hi Jakub,
> 
> Material is net-next. However I don't understand why it should be
> without "Fixes" in such case?
> 
> The material going to current release (RC, so I understood: net), should
> fix only issues introduced in current merge window. Linus made it clear
> several times.

That's absolutely not correct at all.  Bug fixes are always appropriate.
No matter when it is during the merge window.

Maybe you're thinking of the opposite thing where people hoard fixes in
linux-next which are marked for stable.

https://lwn.net/Articles/559113/

    "More importantly: a lot of the patches marked as being for the
     stable tree go into the mainline during the merge window. In many
     cases, that means that the subsystem maintainer held onto the
     patches for some time — months, perhaps — rather than pushing them
     to Linus for a later -rc release. If the patches are important
     enough to go into the stable tree, Greg asked, why are they not
     going to Linus immediately?"

The other thing which annoys me (okay this hasn't happened in probably
five years, but it *used* to annoy me :P) is when people merge code into
linux-next a week before the merge window opens and then it's like we
can't fix basic bugs because times up.  "The merge window is about to
open and it's just a memory leak so we'll push this out for the next
release".

regards,
dan carpenter