* [PATCH] net/unix: fix logic about sk_peek_offset
@ 2015-10-01 21:05 Andrey Vagin
2015-10-02 9:04 ` Andrey Vagin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Andrey Vagin @ 2015-10-01 21:05 UTC (permalink / raw)
To: netdev; +Cc: Andrey Vagin, David S. Miller, Eric Dumazet, Aaron Conole
From: Andrey Vagin <avagin@openvz.org>
Now send with MSG_PEEK can return data from multiple SKBs.
Unfortunately we take into account the peek offset for each skb,
that is wrong. We need to apply the peek offset only once.
In addition, the peek offset should be used only if MSG_PEEK is set.
Cc: "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING
Cc: Eric Dumazet <edumazet@google.com> (commit_signer:1/14=7%)
Cc: Aaron Conole <aconole@bytheb.org>
Fixes: 9f389e35674f ("af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag")
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
net/unix/af_unix.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ef31b40..94f6582 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2064,6 +2064,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
goto out;
}
+ if (flags & MSG_PEEK)
+ skip = sk_peek_offset(sk, flags);
+ else
+ skip = 0;
+
do {
int chunk;
struct sk_buff *skb, *last;
@@ -2112,7 +2117,6 @@ unlock:
break;
}
- skip = sk_peek_offset(sk, flags);
while (skip >= unix_skb_len(skb)) {
skip -= unix_skb_len(skb);
last = skb;
@@ -2179,14 +2183,12 @@ unlock:
if (UNIXCB(skb).fp)
scm.fp = scm_fp_dup(UNIXCB(skb).fp);
- if (skip) {
- sk_peek_offset_fwd(sk, chunk);
- skip -= chunk;
- }
+ sk_peek_offset_fwd(sk, chunk);
if (UNIXCB(skb).fp)
break;
+ skip = 0;
last = skb;
last_len = skb->len;
unix_state_lock(sk);
--
2.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net/unix: fix logic about sk_peek_offset
2015-10-01 21:05 [PATCH] net/unix: fix logic about sk_peek_offset Andrey Vagin
@ 2015-10-02 9:04 ` Andrey Vagin
2015-10-02 12:17 ` Aaron Conole
2015-10-04 12:50 ` Aaron Conole
2015-10-05 13:33 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Andrey Vagin @ 2015-10-02 9:04 UTC (permalink / raw)
To: netdev; +Cc: Andrey Vagin, David S. Miller, Eric Dumazet, Aaron Conole
[-- Attachment #1: Type: text/plain, Size: 4685 bytes --]
2015-10-02 0:05 GMT+03:00 Andrey Vagin <avagin@gmail.com>:
> From: Andrey Vagin <avagin@openvz.org>
>
> Now send with MSG_PEEK can return data from multiple SKBs.
>
> Unfortunately we take into account the peek offset for each skb,
> that is wrong. We need to apply the peek offset only once.
>
> In addition, the peek offset should be used only if MSG_PEEK is set.
The attached program can be used to reproduce the bug. It cycles in an
infinite loop without this patch, because recv() always returns data.
658 socketpair(PF_LOCAL, SOCK_STREAM, 0, [3, 4]) = 0
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 sendto(3, "Hello, World!", 13, MSG_DONTWAIT, NULL, 0) = 13
658 setsockopt(4, SOL_SOCKET, 0x2a /* SO_??? */, [0], 4) = 0
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 fstat(1, {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 3), ...}) = 0
658 ioctl(1, TCGETS, 0x7fff54e57b30) = -1 ENOTTY (Inappropriate
ioctl for device)
658 mmap(NULL, 4096, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f3a86827000
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
658 recvfrom(4, "Hello, World!H", 14, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 14
...
>
> Cc: "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING
> Cc: Eric Dumazet <edumazet@google.com> (commit_signer:1/14=7%)
> Cc: Aaron Conole <aconole@bytheb.org>
> Fixes: 9f389e35674f ("af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag")
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
> net/unix/af_unix.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index ef31b40..94f6582 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2064,6 +2064,11 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
> goto out;
> }
>
> + if (flags & MSG_PEEK)
> + skip = sk_peek_offset(sk, flags);
> + else
> + skip = 0;
> +
> do {
> int chunk;
> struct sk_buff *skb, *last;
> @@ -2112,7 +2117,6 @@ unlock:
> break;
> }
>
> - skip = sk_peek_offset(sk, flags);
> while (skip >= unix_skb_len(skb)) {
> skip -= unix_skb_len(skb);
> last = skb;
> @@ -2179,14 +2183,12 @@ unlock:
> if (UNIXCB(skb).fp)
> scm.fp = scm_fp_dup(UNIXCB(skb).fp);
>
> - if (skip) {
> - sk_peek_offset_fwd(sk, chunk);
> - skip -= chunk;
> - }
> + sk_peek_offset_fwd(sk, chunk);
>
> if (UNIXCB(skb).fp)
> break;
>
> + skip = 0;
> last = skb;
> last_len = skb->len;
> unix_state_lock(sk);
> --
> 2.4.3
>
[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 743 bytes --]
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/socket.h>
#define TEST_STRING "Hello, World!"
int main()
{
char buf[15];
int sks[2];
int i, sk, ret;
if (socketpair(AF_UNIX, SOCK_STREAM, 0, sks))
return 1;
sk = sks[0];
for (i = 0; i < 10; i++) {
if (send(sk, TEST_STRING, sizeof(TEST_STRING) - 1, MSG_DONTWAIT) < 0)
return 1;
}
sk = sks[1];
ret = 0;
if (setsockopt(sk, SOL_SOCKET, SO_PEEK_OFF, &ret, sizeof(int)))
return 1;
while (1) {
int ret;
ret = recv(sk, buf, sizeof(buf) - 1, MSG_DONTWAIT | MSG_PEEK);
if (ret < 0) {
if (errno == EAGAIN)
break;
return 1;
}
if (ret == 0)
break;
buf[ret] = 0;
printf("msg: %s\n", buf);
}
return 0;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/unix: fix logic about sk_peek_offset
2015-10-02 9:04 ` Andrey Vagin
@ 2015-10-02 12:17 ` Aaron Conole
2015-10-03 19:48 ` Andrey Vagin
0 siblings, 1 reply; 6+ messages in thread
From: Aaron Conole @ 2015-10-02 12:17 UTC (permalink / raw)
To: Andrey Vagin; +Cc: netdev, David S. Miller, Eric Dumazet
Andrey Vagin <avagin@openvz.org> writes:
> 2015-10-02 0:05 GMT+03:00 Andrey Vagin <avagin@gmail.com>:
>> From: Andrey Vagin <avagin@openvz.org>
>>
>> Now send with MSG_PEEK can return data from multiple SKBs.
>>
>> Unfortunately we take into account the peek offset for each skb,
>> that is wrong. We need to apply the peek offset only once.
>>
>> In addition, the peek offset should be used only if MSG_PEEK is set.
<<snip>>
Agreed here, the behavior is off in the peek offset case.
I was unable to apply your patch, for some reason, but will manually
try to apply it and test.
Thanks very much!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/unix: fix logic about sk_peek_offset
2015-10-02 12:17 ` Aaron Conole
@ 2015-10-03 19:48 ` Andrey Vagin
0 siblings, 0 replies; 6+ messages in thread
From: Andrey Vagin @ 2015-10-03 19:48 UTC (permalink / raw)
To: Aaron Conole; +Cc: netdev, David S. Miller, Eric Dumazet
2015-10-02 15:17 GMT+03:00 Aaron Conole <aconole@bytheb.org>:
> Andrey Vagin <avagin@openvz.org> writes:
>
>> 2015-10-02 0:05 GMT+03:00 Andrey Vagin <avagin@gmail.com>:
>>> From: Andrey Vagin <avagin@openvz.org>
>>>
>>> Now send with MSG_PEEK can return data from multiple SKBs.
>>>
>>> Unfortunately we take into account the peek offset for each skb,
>>> that is wrong. We need to apply the peek offset only once.
>>>
>>> In addition, the peek offset should be used only if MSG_PEEK is set.
> <<snip>>
> Agreed here, the behavior is off in the peek offset case.
>
> I was unable to apply your patch, for some reason, but will manually
> try to apply it and test.
It's strange. I've checked that this patch can be applied to
davem/net.git and next/linux-next.git,
[avagin@laptop linux-next]$ git checkout net/master
HEAD is now at 36f8daf... Merge tag 'mmc-v4.3-rc3' of
git://git.linaro.org/people/ulf.hansson/mmc
[avagin@laptop linux-next]$ wget https://patchwork.ozlabs.org/patch/525310/mbox/
--2015-10-03 22:46:44-- https://patchwork.ozlabs.org/patch/525310/mbox/
Resolving patchwork.ozlabs.org (patchwork.ozlabs.org)...
103.22.144.67, 2401:3900:2:1::2
Connecting to patchwork.ozlabs.org
(patchwork.ozlabs.org)|103.22.144.67|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/plain]
Saving to: ‘index.html’
index.html [ <=>
] 2.06K
--.-KB/s in 0s
2015-10-03 22:46:47 (122 MB/s) - ‘index.html’ saved [2106]
[avagin@laptop linux-next]$ git am index.html
Applying: net/unix: fix logic about sk_peek_offset
>
> Thanks very much!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/unix: fix logic about sk_peek_offset
2015-10-01 21:05 [PATCH] net/unix: fix logic about sk_peek_offset Andrey Vagin
2015-10-02 9:04 ` Andrey Vagin
@ 2015-10-04 12:50 ` Aaron Conole
2015-10-05 13:33 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Aaron Conole @ 2015-10-04 12:50 UTC (permalink / raw)
To: Andrey Vagin; +Cc: netdev, Andrey Vagin, David S. Miller, Eric Dumazet
Andrey Vagin <avagin@gmail.com> writes:
> From: Andrey Vagin <avagin@openvz.org>
>
> Now send with MSG_PEEK can return data from multiple SKBs.
>
> Unfortunately we take into account the peek offset for each skb,
> that is wrong. We need to apply the peek offset only once.
>
> In addition, the peek offset should be used only if MSG_PEEK is set.
>
> Cc: "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING
> Cc: Eric Dumazet <edumazet@google.com> (commit_signer:1/14=7%)
> Cc: Aaron Conole <aconole@bytheb.org>
> Fixes: 9f389e35674f ("af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag")
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
Tested-by: Aaron Conole <aconole@bytheb.org>
Thanks again!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/unix: fix logic about sk_peek_offset
2015-10-01 21:05 [PATCH] net/unix: fix logic about sk_peek_offset Andrey Vagin
2015-10-02 9:04 ` Andrey Vagin
2015-10-04 12:50 ` Aaron Conole
@ 2015-10-05 13:33 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2015-10-05 13:33 UTC (permalink / raw)
To: avagin; +Cc: netdev, avagin, edumazet, aconole
From: Andrey Vagin <avagin@gmail.com>
Date: Fri, 2 Oct 2015 00:05:36 +0300
> From: Andrey Vagin <avagin@openvz.org>
>
> Now send with MSG_PEEK can return data from multiple SKBs.
>
> Unfortunately we take into account the peek offset for each skb,
> that is wrong. We need to apply the peek offset only once.
>
> In addition, the peek offset should be used only if MSG_PEEK is set.
>
> Cc: "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING
> Cc: Eric Dumazet <edumazet@google.com> (commit_signer:1/14=7%)
> Cc: Aaron Conole <aconole@bytheb.org>
> Fixes: 9f389e35674f ("af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag")
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-05 13:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-01 21:05 [PATCH] net/unix: fix logic about sk_peek_offset Andrey Vagin
2015-10-02 9:04 ` Andrey Vagin
2015-10-02 12:17 ` Aaron Conole
2015-10-03 19:48 ` Andrey Vagin
2015-10-04 12:50 ` Aaron Conole
2015-10-05 13:33 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).