| Message ID | 20240410171016.7621-3-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 22dd70eb2c3d754862964377a75abafd3167346b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | af_unix: Fix MSG_OOB bugs with MSG_PEEK. | expand |
The proposed fix is not the correct fix as among other things it does not allow going pass the OOB if data is present. TCP allows that. I have attached a patch that fixes this issue and other issues that I encountered in my testing. I compared TCP. Shoaib On 4/10/24 10:10, Kuniyuki Iwashima wrote: > Currently, we can read OOB data without MSG_OOB by using MSG_PEEK > when OOB data is sitting on the front row, which is apparently > wrong. > > >>> from socket import * > >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM) > >>> c1.send(b'a', MSG_OOB) > 1 > >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT) > b'a' > > If manage_oob() is called when no data has been copied, we only > check if the socket enables SO_OOBINLINE or MSG_PEEK is not used. > Otherwise, the skb is returned as is. > > However, here we should return NULL if MSG_PEEK is set and no data > has been copied. > > Also, in such a case, we should not jump to the redo label because > we will be caught in the loop and hog the CPU until normal data > comes in. > > Then, we need to handle skb == NULL case with the if-clause below > the manage_oob() block. > > With this patch: > > >>> from socket import * > >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM) > >>> c1.send(b'a', MSG_OOB) > 1 > >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT) > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > BlockingIOError: [Errno 11] Resource temporarily unavailable > > Fixes: 314001f0bf92 ("af_unix: Add OOB support") > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > net/unix/af_unix.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index f297320438bf..9a6ad5974dff 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -2663,7 +2663,9 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, > WRITE_ONCE(u->oob_skb, NULL); > consume_skb(skb); > } > - } else if (!(flags & MSG_PEEK)) { > + } else if (flags & MSG_PEEK) { > + skb = NULL; > + } else { > skb_unlink(skb, &sk->sk_receive_queue); > WRITE_ONCE(u->oob_skb, NULL); > if (!WARN_ON_ONCE(skb_unref(skb))) > @@ -2745,11 +2747,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, > #if IS_ENABLED(CONFIG_AF_UNIX_OOB) > if (skb) { > skb = manage_oob(skb, sk, flags, copied); > - if (!skb) { > + if (!skb && copied) { > unix_state_unlock(sk); > - if (copied) > - break; > - goto redo; > + break; > } > } > #endif
From: Rao Shoaib <rao.shoaib@oracle.com> Date: Tue, 16 Apr 2024 13:11:09 -0700 > The proposed fix is not the correct fix as among other things it does > not allow going pass the OOB if data is present. TCP allows that. Ugh, exactly. But the behaviour was broken initially, so the tag is Fixes: 314001f0bf92 ("af_unix: Add OOB support") TCP: ---8<--- >>> from socket import * >>> s = socket() >>> s.listen() >>> c1 = socket() >>> c1.connect(s.getsockname()) >>> c2, _ = s.accept() >>> >>> c1.send(b'h', MSG_OOB) 1 >>> c1.send(b'ello') 4 >>> c2.recv(5, MSG_PEEK) b'ello' ---8<--- Latest net.git ---8<--- >>> from socket import * >>> c1, c2 = socketpair(AF_UNIX) >>> c1.send(b'h', MSG_OOB) 1 >>> c1.send(b'ello') 4 >>> >>> c2.recv(5, MSG_PEEK) ^C ---8<--- 314001f0bf92: ---8<--- >>> from socket import * >>> c1, c2 = socketpair(AF_UNIX) >>> c1.send(b'h', MSG_OOB) 1 >>> c1.send(b'ello') 4 >>> c2.recv(5, MSG_PEEK) b'hello' ---8<--- > > I have attached a patch that fixes this issue and other issues that I > encountered in my testing. I compared TCP. Could you post patches formally on top of the latest net.git ? It seems one of my patch is squashed. Also, please note that one patch should fix one issue. The change in queue_oob() should be another patch. Thanks!
On 4/16/24 13:51, Kuniyuki Iwashima wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > Date: Tue, 16 Apr 2024 13:11:09 -0700 >> The proposed fix is not the correct fix as among other things it does >> not allow going pass the OOB if data is present. TCP allows that. > > Ugh, exactly. > > But the behaviour was broken initially, so the tag is > > Fixes: 314001f0bf92 ("af_unix: Add OOB support") > Where is this requirement listed? > Could you post patches formally on top of the latest net.git ? > It seems one of my patch is squashed. I pulled in last night, your last fix has not yet made it (I think) [rshoaib@turbo-2 linux_oob]$ git describe v6.9-rc4-32-gbf541423b785 > > Also, please note that one patch should fix one issue. > The change in queue_oob() should be another patch. > I was just responding to your email. I was not sure if you wanted to modify your fix. If you prefer I submit the patches, I will later. Shoaib
From: Rao Shoaib <rao.shoaib@oracle.com> Date: Tue, 16 Apr 2024 14:34:20 -0700 > On 4/16/24 13:51, Kuniyuki Iwashima wrote: > > From: Rao Shoaib <rao.shoaib@oracle.com> > > Date: Tue, 16 Apr 2024 13:11:09 -0700 > >> The proposed fix is not the correct fix as among other things it does > >> not allow going pass the OOB if data is present. TCP allows that. > > > > Ugh, exactly. > > > > But the behaviour was broken initially, so the tag is > > > > Fixes: 314001f0bf92 ("af_unix: Add OOB support") > > > > Where is this requirement listed? Please start with these docs. https://docs.kernel.org/process/submitting-patches.html https://docs.kernel.org/process/maintainer-netdev.html > > > > Could you post patches formally on top of the latest net.git ? > > It seems one of my patch is squashed. > > I pulled in last night, your last fix has not yet made it (I think) > > [rshoaib@turbo-2 linux_oob]$ git describe > v6.9-rc4-32-gbf541423b785 Probably you are using another git tree or branch. Networking subsystem uses net.git for fixes and net-next.git for new features as written in the 2nd doc above. My patch landed on 4 days ago at least. https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=283454c8a123072e5c386a5a2b5fc576aa455b6f Also you should receive this email. https://lore.kernel.org/netdev/171297422982.31124.3409808601326947596.git-patchwork-notify@kernel.org/ > > > > > Also, please note that one patch should fix one issue. > > The change in queue_oob() should be another patch. > > > > I was just responding to your email. I was not sure if you wanted to > modify your fix. If you prefer I submit the patches, I will later. As I said, my fix is already in net.git, so you can post a separte patch based on net.git/main.
On 4/16/24 14:47, Kuniyuki Iwashima wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > Date: Tue, 16 Apr 2024 14:34:20 -0700 >> On 4/16/24 13:51, Kuniyuki Iwashima wrote: >>> From: Rao Shoaib <rao.shoaib@oracle.com> >>> Date: Tue, 16 Apr 2024 13:11:09 -0700 >>>> The proposed fix is not the correct fix as among other things it does >>>> not allow going pass the OOB if data is present. TCP allows that. >>> >>> Ugh, exactly. >>> >>> But the behaviour was broken initially, so the tag is >>> >>> Fixes: 314001f0bf92 ("af_unix: Add OOB support") >>> >> >> Where is this requirement listed? > > Please start with these docs. > https://urldefense.com/v3/__https://docs.kernel.org/process/submitting-patches.html__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUA5Yikc2A$ > https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer-netdev.html__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUAdoz3l7w$ > > That is a suggestion. I see commits in even af_unix.c which do not follow that convention. They just mention what the fix is about. In this case it is implied. I am not opposed specifying it but it seems it's optional. >> >> >>> Could you post patches formally on top of the latest net.git ? >>> It seems one of my patch is squashed. >> >> I pulled in last night, your last fix has not yet made it (I think) >> >> [rshoaib@turbo-2 linux_oob]$ git describe >> v6.9-rc4-32-gbf541423b785 > > Probably you are using another git tree or branch. > > Networking subsystem uses net.git for fixes and net-next.git for new > features as written in the 2nd doc above. > > My patch landed on 4 days ago at least. > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=283454c8a123072e5c386a5a2b5fc576aa455b6f__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUA32gMtng$ > > Also you should receive this email. > https://urldefense.com/v3/__https://lore.kernel.org/netdev/171297422982.31124.3409808601326947596.git-patchwork-notify@kernel.org/__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUAnOykbCY$ > > >> >>> >>> Also, please note that one patch should fix one issue. >>> The change in queue_oob() should be another patch. >>> >> >> I was just responding to your email. I was not sure if you wanted to >> modify your fix. If you prefer I submit the patches, I will later. > > As I said, my fix is already in net.git, so you can post a separte > patch based on net.git/main. > I used the latest from Linus. I will submit the patches later. Shoaib
From: Rao Shoaib <rao.shoaib@oracle.com> Date: Tue, 16 Apr 2024 15:01:15 -0700 > On 4/16/24 14:47, Kuniyuki Iwashima wrote: > > From: Rao Shoaib <rao.shoaib@oracle.com> > > Date: Tue, 16 Apr 2024 14:34:20 -0700 > >> On 4/16/24 13:51, Kuniyuki Iwashima wrote: > >>> From: Rao Shoaib <rao.shoaib@oracle.com> > >>> Date: Tue, 16 Apr 2024 13:11:09 -0700 > >>>> The proposed fix is not the correct fix as among other things it does > >>>> not allow going pass the OOB if data is present. TCP allows that. > >>> > >>> Ugh, exactly. > >>> > >>> But the behaviour was broken initially, so the tag is > >>> > >>> Fixes: 314001f0bf92 ("af_unix: Add OOB support") > >>> > >> > >> Where is this requirement listed? > > > > Please start with these docs. > > https://urldefense.com/v3/__https://docs.kernel.org/process/submitting-patches.html__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUA5Yikc2A$ > > https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer-netdev.html__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUAdoz3l7w$ > > > > > That is a suggestion. I see commits in even af_unix.c which do not > follow that convention. They just mention what the fix is about. In this > case it is implied. > > I am not opposed specifying it but it seems it's optional. You want to read the 2nd doc. 1.1 tl;dr for fixes the Fixes: tag is required, regardless of the tree
On 4/16/24 15:10, Kuniyuki Iwashima wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > Date: Tue, 16 Apr 2024 15:01:15 -0700 >> On 4/16/24 14:47, Kuniyuki Iwashima wrote: >>> From: Rao Shoaib <rao.shoaib@oracle.com> >>> Date: Tue, 16 Apr 2024 14:34:20 -0700 >>>> On 4/16/24 13:51, Kuniyuki Iwashima wrote: >>>>> From: Rao Shoaib <rao.shoaib@oracle.com> >>>>> Date: Tue, 16 Apr 2024 13:11:09 -0700 >>>>>> The proposed fix is not the correct fix as among other things it does >>>>>> not allow going pass the OOB if data is present. TCP allows that. >>>>> >>>>> Ugh, exactly. >>>>> >>>>> But the behaviour was broken initially, so the tag is >>>>> >>>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support") >>>>> >>>> >>>> Where is this requirement listed? >>> >>> Please start with these docs. >>> https://urldefense.com/v3/__https://docs.kernel.org/process/submitting-patches.html__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUA5Yikc2A$ >>> https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer-netdev.html__;!!ACWV5N9M2RV99hQ!PswtQoZm7r5MGnH8pv3OewI_PvmSRJb29YcA0pnVOzuu8T3xvWlw4lLlLzFhzn6uO2lo0bUAdoz3l7w$ >>> >>> >> That is a suggestion. I see commits in even af_unix.c which do not >> follow that convention. They just mention what the fix is about. In this >> case it is implied. >> >> I am not opposed specifying it but it seems it's optional. > > You want to read the 2nd doc. > > 1.1 tl;dr > for fixes the Fixes: tag is required, regardless of the tree Thanks will do. Shoaib
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index f297320438bf..9a6ad5974dff 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2663,7 +2663,9 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, WRITE_ONCE(u->oob_skb, NULL); consume_skb(skb); } - } else if (!(flags & MSG_PEEK)) { + } else if (flags & MSG_PEEK) { + skb = NULL; + } else { skb_unlink(skb, &sk->sk_receive_queue); WRITE_ONCE(u->oob_skb, NULL); if (!WARN_ON_ONCE(skb_unref(skb))) @@ -2745,11 +2747,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, #if IS_ENABLED(CONFIG_AF_UNIX_OOB) if (skb) { skb = manage_oob(skb, sk, flags, copied); - if (!skb) { + if (!skb && copied) { unix_state_unlock(sk); - if (copied) - break; - goto redo; + break; } } #endif