* [PATCH net] nfc: avoid potential race condition
@ 2021-09-23 6:50 Dan Carpenter
2021-09-23 7:26 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Dan Carpenter @ 2021-09-23 6:50 UTC (permalink / raw)
To: Krzysztof Kozlowski, Samuel Ortiz
Cc: David S. Miller, Jakub Kicinski, John W. Linville, netdev,
kernel-janitors
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net] nfc: avoid potential race condition 2021-09-23 6:50 [PATCH net] nfc: avoid potential race condition Dan Carpenter @ 2021-09-23 7:26 ` Krzysztof Kozlowski 2021-09-23 12:22 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2021-09-23 7:26 UTC (permalink / raw) To: Dan Carpenter, Samuel Ortiz Cc: David S. Miller, Jakub Kicinski, John W. Linville, netdev, kernel-janitors 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); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-23 7:26 ` Krzysztof Kozlowski @ 2021-09-23 12:22 ` Dan Carpenter 2021-09-24 8:21 ` Krzysztof Kozlowski 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-09-23 12:22 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Samuel Ortiz, David S. Miller, Jakub Kicinski, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-23 12:22 ` Dan Carpenter @ 2021-09-24 8:21 ` Krzysztof Kozlowski 2021-09-24 20:14 ` Jakub Kicinski 0 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2021-09-24 8:21 UTC (permalink / raw) To: Dan Carpenter Cc: Samuel Ortiz, David S. Miller, Jakub Kicinski, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-24 8:21 ` Krzysztof Kozlowski @ 2021-09-24 20:14 ` Jakub Kicinski 2021-09-27 7:44 ` Krzysztof Kozlowski 2021-09-27 12:14 ` Dan Carpenter 0 siblings, 2 replies; 13+ messages in thread From: Jakub Kicinski @ 2021-09-24 20:14 UTC (permalink / raw) To: Krzysztof Kozlowski, Dan Carpenter Cc: Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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! :) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-24 20:14 ` Jakub Kicinski @ 2021-09-27 7:44 ` Krzysztof Kozlowski 2021-09-27 14:26 ` Jakub Kicinski 2021-09-27 14:49 ` Dan Carpenter 2021-09-27 12:14 ` Dan Carpenter 1 sibling, 2 replies; 13+ messages in thread From: Krzysztof Kozlowski @ 2021-09-27 7:44 UTC (permalink / raw) To: Jakub Kicinski, Dan Carpenter Cc: Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-27 7:44 ` Krzysztof Kozlowski @ 2021-09-27 14:26 ` Jakub Kicinski 2021-09-27 14:58 ` Krzysztof Kozlowski 2021-09-27 14:49 ` Dan Carpenter 1 sibling, 1 reply; 13+ messages in thread From: Jakub Kicinski @ 2021-09-27 14:26 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Dan Carpenter, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-27 14:26 ` Jakub Kicinski @ 2021-09-27 14:58 ` Krzysztof Kozlowski 2021-09-27 15:13 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2021-09-27 14:58 UTC (permalink / raw) To: Jakub Kicinski Cc: Dan Carpenter, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-27 14:58 ` Krzysztof Kozlowski @ 2021-09-27 15:13 ` Dan Carpenter 2021-09-27 15:27 ` Krzysztof Kozlowski 0 siblings, 1 reply; 13+ messages in thread From: Dan Carpenter @ 2021-09-27 15:13 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Jakub Kicinski, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-27 15:13 ` Dan Carpenter @ 2021-09-27 15:27 ` Krzysztof Kozlowski 2021-09-27 15:38 ` Dan Carpenter 0 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2021-09-27 15:27 UTC (permalink / raw) To: Dan Carpenter Cc: Jakub Kicinski, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-27 15:27 ` Krzysztof Kozlowski @ 2021-09-27 15:38 ` Dan Carpenter 0 siblings, 0 replies; 13+ messages in thread From: Dan Carpenter @ 2021-09-27 15:38 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Jakub Kicinski, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-27 7:44 ` Krzysztof Kozlowski 2021-09-27 14:26 ` Jakub Kicinski @ 2021-09-27 14:49 ` Dan Carpenter 1 sibling, 0 replies; 13+ messages in thread From: Dan Carpenter @ 2021-09-27 14:49 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Jakub Kicinski, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] nfc: avoid potential race condition 2021-09-24 20:14 ` Jakub Kicinski 2021-09-27 7:44 ` Krzysztof Kozlowski @ 2021-09-27 12:14 ` Dan Carpenter 1 sibling, 0 replies; 13+ messages in thread From: Dan Carpenter @ 2021-09-27 12:14 UTC (permalink / raw) To: Jakub Kicinski Cc: Krzysztof Kozlowski, Samuel Ortiz, David S. Miller, John W. Linville, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-27 15:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-23 6:50 [PATCH net] nfc: avoid potential race condition Dan Carpenter 2021-09-23 7:26 ` Krzysztof Kozlowski 2021-09-23 12:22 ` Dan Carpenter 2021-09-24 8:21 ` Krzysztof Kozlowski 2021-09-24 20:14 ` Jakub Kicinski 2021-09-27 7:44 ` Krzysztof Kozlowski 2021-09-27 14:26 ` Jakub Kicinski 2021-09-27 14:58 ` Krzysztof Kozlowski 2021-09-27 15:13 ` Dan Carpenter 2021-09-27 15:27 ` Krzysztof Kozlowski 2021-09-27 15:38 ` Dan Carpenter 2021-09-27 14:49 ` Dan Carpenter 2021-09-27 12:14 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.