[PATCH net] nfc: avoid potential race condition

[PATCH net] nfc: avoid potential race condition

[Dan Carpenter]

This from static analysis inspired by CVE-2021-26708 where there was a
race condition because it didn't lock_sock(sk) before saving
"vsk->transport".  Here it is saving "llcp_sock->local" but the concept
is the same that it needs to take the lock first.

Fixes: 00e856db49bb ("NFC: llcp: Fall back to local values when getting socket options")
Fixes: d646960f7986 ("NFC: Initial LLCP support")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 net/nfc/llcp_sock.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index 6cfd30fc0798..74f4209c7144 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -314,14 +314,16 @@ static int nfc_llcp_getsockopt(struct socket *sock, int level, int optname,
 	if (get_user(len, optlen))
 		return -EFAULT;
 
-	local = llcp_sock->local;
-	if (!local)
-		return -ENODEV;
-
 	len = min_t(u32, len, sizeof(u32));
 
 	lock_sock(sk);
 
+	local = llcp_sock->local;
+	if (!local) {
+		release_sock(sk);
+		return -ENODEV;
+	}
+
 	switch (optname) {
 	case NFC_LLCP_RW:
 		rw = llcp_sock->rw > LLCP_MAX_RW ? local->rw : llcp_sock->rw;
@@ -598,14 +600,15 @@ static int llcp_sock_release(struct socket *sock)
 
 	pr_debug("%p\n", sk);
 
+	lock_sock(sk);
+
 	local = llcp_sock->local;
 	if (local == NULL) {
+		release_sock(sk);
 		err = -ENODEV;
 		goto out;
 	}
 
-	lock_sock(sk);
-
 	/* Send a DISC */
 	if (sk->sk_state == LLCP_CONNECTED)
 		nfc_llcp_send_disconnect(llcp_sock);
-- 
2.20.1

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

[Krzysztof Kozlowski]

On 23/09/2021 08:50, Dan Carpenter wrote:
> This from static analysis inspired by CVE-2021-26708 where there was a
> race condition because it didn't lock_sock(sk) before saving
> "vsk->transport".  Here it is saving "llcp_sock->local" but the concept
> is the same that it needs to take the lock first.

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?

The code except this looks reasonable and since writer protects
llcp_sock->local(), the reader I guess should do it as well... just
wondering whether this is a real issue.


Best regards,
Krzysztof

> 
> Fixes: 00e856db49bb ("NFC: llcp: Fall back to local values when getting socket options")
> Fixes: d646960f7986 ("NFC: Initial LLCP support")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  net/nfc/llcp_sock.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index 6cfd30fc0798..74f4209c7144 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -314,14 +314,16 @@ static int nfc_llcp_getsockopt(struct socket *sock, int level, int optname,
>  	if (get_user(len, optlen))
>  		return -EFAULT;
>  
> -	local = llcp_sock->local;
> -	if (!local)
> -		return -ENODEV;
> -
>  	len = min_t(u32, len, sizeof(u32));
>  
>  	lock_sock(sk);
>  
> +	local = llcp_sock->local;
> +	if (!local) {
> +		release_sock(sk);
> +		return -ENODEV;
> +	}
> +
>  	switch (optname) {
>  	case NFC_LLCP_RW:
>  		rw = llcp_sock->rw > LLCP_MAX_RW ? local->rw : llcp_sock->rw;
> @@ -598,14 +600,15 @@ static int llcp_sock_release(struct socket *sock)
>  
>  	pr_debug("%p\n", sk);
>  
> +	lock_sock(sk);
> +
>  	local = llcp_sock->local;
>  	if (local == NULL) {
> +		release_sock(sk);
>  		err = -ENODEV;
>  		goto out;
>  	}
>  
> -	lock_sock(sk);
> -
>  	/* Send a DISC */
>  	if (sk->sk_state == LLCP_CONNECTED)
>  		nfc_llcp_send_disconnect(llcp_sock);
> 

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

[Dan Carpenter]

On Thu, Sep 23, 2021 at 09:26:51AM +0200, Krzysztof Kozlowski wrote:
> On 23/09/2021 08:50, Dan Carpenter wrote:
> > This from static analysis inspired by CVE-2021-26708 where there was a
> > race condition because it didn't lock_sock(sk) before saving
> > "vsk->transport".  Here it is saving "llcp_sock->local" but the concept
> > is the same that it needs to take the lock first.
> 
> 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).

regards,
dan carpenter

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

[Krzysztof Kozlowski]

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:
>>> This from static analysis inspired by CVE-2021-26708 where there was a
>>> race condition because it didn't lock_sock(sk) before saving
>>> "vsk->transport".  Here it is saving "llcp_sock->local" but the concept
>>> is the same that it needs to take the lock first.
>>
>> 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>


Best regards,
Krzysztof

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

[Jakub Kicinski]

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! :)

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

[Krzysztof Kozlowski]

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.

The issue here was introduced long time ago, not in current merge
window, however it is still an issue to fix. It's still a bug which
should have a commit with "Fixes" for all the stable tress and
downstream distros relying on stable kernels. Also for some statistics
on LWN.


Best regards,
Krzysztof

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

[Dan Carpenter]

On Fri, Sep 24, 2021 at 01:14:41PM -0700, 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! :)

This should be ported to stable.  The race is condition is real because
->release() can race with itself.  I don't know if expliotable or not
beyond just a denial of service.

regards,
dan carpenter

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

[Jakub Kicinski]

On Mon, 27 Sep 2021 09:44:08 +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:  
> >> 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.

Oh, really? I've never heard about this rule, would you be able to dig
up references?

This strikes me as odd, most fixes we merge are for previous releases.
In fact when I write -rc pull requests to Linus I break them down by
current release vs previous - and he never complained.

> The issue here was introduced long time ago, not in current merge
> window, however it is still an issue to fix. It's still a bug which
> should have a commit with "Fixes" for all the stable tress and
> downstream distros relying on stable kernels. Also for some statistics
> on LWN.

Stable will not pull the commit from net-next, tho. Stable is more
restrictive than rc (or at least so I think) so "we want it in stable,
please merge it to net-next" does not compute with the preconceptions 
I have.

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

[Dan Carpenter]

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

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

[Krzysztof Kozlowski]

On 27/09/2021 16:26, Jakub Kicinski wrote:
> On Mon, 27 Sep 2021 09:44:08 +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:  
>>>> 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.
> 
> Oh, really? I've never heard about this rule, would you be able to dig
> up references?

Not that easy to go through thousands of emails, but I'll try:

"One thing that does bother him is developers who send him fixes in the
-rc2 or -rc3 time frame for things that never worked in the first place.
If something never worked, then the fact that it doesn't work now is not
a regression, so the fixes should just wait for the next merge window.
Those fixes are, after all, essentially development work."
https://lwn.net/Articles/705245/

"The rc stuff is for regressions, and for things that actually are
nasty problems (security, keeping people from getting work done)."
https://lore.kernel.org/lkml/CA+55aFyn1matXDTkeDA1d2+tHBSVkBvS5kP-7Ngh86=uut+yyg@mail.gmail.com/

"NONE of this seems really to be appropriate for this stage. It doesn't
fix regressions, it doesn't fix security stuff, it doesn't really fix
major oopses."
https://lore.kernel.org/lkml/CA+55aFyvW38WU93CqegHiKt-ReO8yNfAOUGZRFGY3-Aq0UsETw@mail.gmail.com/

"No, I definitely don't want anything now unless it's a major
regression or security issue. Other stuff can wait until the merge
window and perhaps be marked for stable if required. That way they'll
get testing."
https://lore.kernel.org/lkml/CA+55aFyWcdmy3ACAWmRq70kQDpJ3bkjv1nROd1Gvab1Aa-GHqA@mail.gmail.com/

Linux seems to be flexible around that as he also pulls several fixes
which were broken before.


> This strikes me as odd, most fixes we merge are for previous releases.
> In fact when I write -rc pull requests to Linus I break them down by
> current release vs previous - and he never complained.

True, I noticed it. Maybe the rule is much less stricter than I
understood it.

> 
>> The issue here was introduced long time ago, not in current merge
>> window, however it is still an issue to fix. It's still a bug which
>> should have a commit with "Fixes" for all the stable tress and
>> downstream distros relying on stable kernels. Also for some statistics
>> on LWN.
> 
> Stable will not pull the commit from net-next, tho. Stable is more
> restrictive than rc (or at least so I think) so "we want it in stable,
> please merge it to net-next" does not compute with the preconceptions 
> I have.

There is no single need for stable to pull the commit from net-next. The
net-next commits will reach the Linus' tree sometime in the future (next
merge window) and then it will go to stable. That's the process. No need
to push such fix earlier, in a rush, for something broken long time ago
and not being a significant regression or bug.


Best regards,
Krzysztof

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

[Dan Carpenter]

On Mon, Sep 27, 2021 at 04:58:45PM +0200, Krzysztof Kozlowski wrote:
> On 27/09/2021 16:26, Jakub Kicinski wrote:
> > On Mon, 27 Sep 2021 09:44:08 +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:  
> >>>> 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.
> > 
> > Oh, really? I've never heard about this rule, would you be able to dig
> > up references?
> 
> Not that easy to go through thousands of emails, but I'll try:
> 
> "One thing that does bother him is developers who send him fixes in the
> -rc2 or -rc3 time frame for things that never worked in the first place.
> If something never worked, then the fact that it doesn't work now is not
> a regression, so the fixes should just wait for the next merge window.
> Those fixes are, after all, essentially development work."
> https://lwn.net/Articles/705245/ 

Yes.  He's talking about fixes to new features which don't work at all.

I once discovered a module that had a bug in probe() and it had never
once been able to probe without crashing.  It had been in the kernel for
ten years.  The developer was like, "Yeah.  We knew it was crap and
wanted to delete it but that was before git and Linus lost the patch."

Anyway, this is a security bug (DoS at the minimum) so it should be
merged into net and set to stable.

regards,
dan carpenter

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

[Krzysztof Kozlowski]

On 27/09/2021 17:13, Dan Carpenter wrote:
> On Mon, Sep 27, 2021 at 04:58:45PM +0200, Krzysztof Kozlowski wrote:
>> On 27/09/2021 16:26, Jakub Kicinski wrote:
>>> On Mon, 27 Sep 2021 09:44:08 +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:  
>>>>>> 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.
>>>
>>> Oh, really? I've never heard about this rule, would you be able to dig
>>> up references?
>>
>> Not that easy to go through thousands of emails, but I'll try:
>>
>> "One thing that does bother him is developers who send him fixes in the
>> -rc2 or -rc3 time frame for things that never worked in the first place.
>> If something never worked, then the fact that it doesn't work now is not
>> a regression, so the fixes should just wait for the next merge window.
>> Those fixes are, after all, essentially development work."
>> https://lwn.net/Articles/705245/ 
> 
> Yes.  He's talking about fixes to new features which don't work at all.

Here yes. In other places, it's narrower:
"The rc stuff is for regressions, and for things that actually are
nasty problems (security, keeping people from getting work done)."

> 
> I once discovered a module that had a bug in probe() and it had never
> once been able to probe without crashing.  It had been in the kernel for
> ten years.  The developer was like, "Yeah.  We knew it was crap and
> wanted to delete it but that was before git and Linus lost the patch."
>
> Anyway, this is a security bug (DoS at the minimum) so it should be
> merged into net and set to stable.

I don't mind, just take in mind that Sasha Levin was also pointing out
that quality of fixes applied for RC is poor and usually does not
receive proper testing or settle time.

Someone tested this fix? I did not.

Best regards,
Krzysztof

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

[Dan Carpenter]

On Mon, Sep 27, 2021 at 05:27:18PM +0200, Krzysztof Kozlowski wrote:
> I don't mind, just take in mind that Sasha Levin was also pointing out
> that quality of fixes applied for RC is poor and usually does not
> receive proper testing or settle time.
> 
> Someone tested this fix? I did not.

I have not tested it.  I don't care when it reaches -stable just so long
as it does eventually.

regards,
dan carpenter