* AF_ALG buggy with sendfile
@ 2013-11-24 17:21 Shawn Landden
2013-11-24 22:00 ` Shawn Landden
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Landden @ 2013-11-24 17:21 UTC (permalink / raw)
To: Linux Kernel Mailing List, netdev, herbert, linux-crypto
[-- Attachment #1: Type: text/plain, Size: 273 bytes --]
If I use sendfile() to send to a accept()ed AF_ALG socket set up for
"hash", I get the wrong
answer, if I read() and then write() I get the right answer. None of
the system calls return an error.
test case attached.
--
---
Shawn Landden
+1 360 389 3001 (SMS preferred)
[-- Attachment #2: af_alg.c --]
[-- Type: text/x-csrc, Size: 976 bytes --]
#include <sys/sendfile.h>
#include <sys/socket.h>
#include <linux/if_alg.h>
#include <stdio.h>
#include <sys/stat.h>
#include <fcntl.h>
int main(void)
{
int opfd;
int tfmfd;
struct sockaddr_alg sa = {
.salg_family = AF_ALG,
.salg_type = "hash",
.salg_name = "sha1"
};
char buf2[10000000];
char buf[20];
int i;
struct stat st;
tfmfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(tfmfd, (struct sockaddr *)&sa, sizeof(sa));
opfd = accept(tfmfd, NULL, 0);
int true = open("/bin/true", O_RDONLY);
fstat(true, &st);
sendfile(opfd, true, NULL, st.st_size);
read(opfd, &buf, 20);
for (i = 0; i < 20; i++) {
printf("%02x", (unsigned char)buf[i]);
}
printf("\n");
lseek(true, 0, SEEK_SET);
read(true, &buf2, st.st_size);
write(opfd, &buf2, st.st_size);
read(opfd, &buf, 20);
for (i = 0; i < 20; i++) {
printf("%02x", (unsigned char)buf[i]);
}
printf("\n");
close(opfd);
close(tfmfd);
return 0;
}
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: AF_ALG buggy with sendfile
2013-11-24 17:21 AF_ALG buggy with sendfile Shawn Landden
@ 2013-11-24 22:00 ` Shawn Landden
2013-11-24 22:04 ` Shawn Landden
2013-11-24 23:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
0 siblings, 2 replies; 17+ messages in thread
From: Shawn Landden @ 2013-11-24 22:00 UTC (permalink / raw)
To: Linux Kernel Mailing List, netdev, herbert, linux-crypto
[-- Attachment #1: Type: text/plain, Size: 500 bytes --]
heres a version of the test case that builds.....
Sorry about that.
On Sun, Nov 24, 2013 at 9:21 AM, Shawn Landden <shawnlandden@gmail.com> wrote:
> If I use sendfile() to send to a accept()ed AF_ALG socket set up for
> "hash", I get the wrong
> answer, if I read() and then write() I get the right answer. None of
> the system calls return an error.
>
> test case attached.
>
> --
>
> ---
> Shawn Landden
> +1 360 389 3001 (SMS preferred)
--
---
Shawn Landden
+1 360 389 3001 (SMS preferred)
[-- Attachment #2: af_alg.c --]
[-- Type: text/x-csrc, Size: 1123 bytes --]
#include <sys/sendfile.h>
#include <sys/socket.h>
#include <linux/if_alg.h>
#include <stdio.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdlib.h>
#include <unistd.h>
int main(void)
{
int opfd;
int tfmfd;
struct sockaddr_alg sa = {
.salg_family = AF_ALG,
.salg_type = "hash",
.salg_name = "sha1"
};
char *buf2;
char buf[20];
int i;
struct stat st;
ssize_t size;
tfmfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(tfmfd, (struct sockaddr *)&sa, sizeof(sa));
opfd = accept(tfmfd, NULL, 0);
int t = open("/bin/true", O_RDONLY);
fstat(t, &st);
size = sendfile(opfd, t, NULL, st.st_size);
if (size != st.st_size)
exit(1);
read(opfd, &buf, 20);
for (i = 0; i < 20; i++) {
printf("%02x", (unsigned char)buf[i]);
}
printf("\n");
lseek(t, 0, SEEK_SET);
buf2 = malloc(st.st_size + 1);
read(t, buf2, st.st_size);
write(opfd, buf2, st.st_size);
read(opfd, &buf, 20);
for (i = 0; i < 20; i++) {
printf("%02x", (unsigned char)buf[i]);
}
printf("\n");
close(opfd);
close(tfmfd);
return 0;
}
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: AF_ALG buggy with sendfile
2013-11-24 22:00 ` Shawn Landden
@ 2013-11-24 22:04 ` Shawn Landden
2013-11-24 23:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
1 sibling, 0 replies; 17+ messages in thread
From: Shawn Landden @ 2013-11-24 22:04 UTC (permalink / raw)
To: Linux Kernel Mailing List, netdev, Herbert Xu, linux-crypto
If you build https://kernel.googlesource.com/pub/scm/network/connman/connman/+/0.80/tools/alg-test.c
from the connman source code and compare the output to coreutils
sha1sum you can see the problem.
shawn@debian-T61:~/git/test$ make connman_afalg
cc connman_afalg.c -o connman_afalg
shawn@debian-T61:~/git/test$ ./connman_afalg /bin/true
send 27080 bytes
recv 20 bytes
45384483cf9cd0d82eba164131795b4807c6d39d /bin/true
shawn@debian-T61:~/git/test$ sha1sum /bin/true
82667ba2ec681d8e55b0ee3b6db2970f9911680d /bin/true
On Sun, Nov 24, 2013 at 2:00 PM, Shawn Landden <shawnlandden@gmail.com> wrote:
> heres a version of the test case that builds.....
>
> Sorry about that.
>
> On Sun, Nov 24, 2013 at 9:21 AM, Shawn Landden <shawnlandden@gmail.com> wrote:
>> If I use sendfile() to send to a accept()ed AF_ALG socket set up for
>> "hash", I get the wrong
>> answer, if I read() and then write() I get the right answer. None of
>> the system calls return an error.
>>
>> test case attached.
>>
>> --
>>
>> ---
>> Shawn Landden
>> +1 360 389 3001 (SMS preferred)
>
>
>
> --
>
> ---
> Shawn Landden
> +1 360 389 3001 (SMS preferred)
--
---
Shawn Landden
+1 360 389 3001 (SMS preferred)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST
2013-11-24 22:00 ` Shawn Landden
2013-11-24 22:04 ` Shawn Landden
@ 2013-11-24 23:42 ` Richard Weinberger
2013-11-25 0:03 ` Shawn Landden
2013-11-25 1:25 ` Eric Dumazet
1 sibling, 2 replies; 17+ messages in thread
From: Richard Weinberger @ 2013-11-24 23:42 UTC (permalink / raw)
To: linux-kernel
Cc: linux-fsdevel, viro, shawnlandden, linux-crypto, netdev, herbert,
Richard Weinberger, Tom Herbert, Eric Dumazet, David S. Miller,
stable
Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
added an internal flag MSG_SENDPAGE_NOTLAST.
We have to ensure that MSG_MORE is also set if we set MSG_SENDPAGE_NOTLAST.
Otherwise users that check against MSG_MORE will not see it.
This fixes sendfile() on AF_ALG.
Cc: Tom Herbert <therbert@google.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: <stable@vger.kernel.org> # 3.4.x
Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
fs/splice.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/splice.c b/fs/splice.c
index 3b7ee65..b93f1b8 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -701,7 +701,7 @@ static int pipe_to_sendpage(struct pipe_inode_info *pipe,
more = (sd->flags & SPLICE_F_MORE) ? MSG_MORE : 0;
if (sd->len < sd->total_len && pipe->nrbufs > 1)
- more |= MSG_SENDPAGE_NOTLAST;
+ more |= MSG_SENDPAGE_NOTLAST | MSG_MORE;
return file->f_op->sendpage(file, buf->page, buf->offset,
sd->len, &pos, more);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST
2013-11-24 23:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
@ 2013-11-25 0:03 ` Shawn Landden
2013-11-25 1:25 ` Eric Dumazet
1 sibling, 0 replies; 17+ messages in thread
From: Shawn Landden @ 2013-11-25 0:03 UTC (permalink / raw)
To: Richard Weinberger
Cc: Linux Kernel Mailing List, linux-fsdevel, viro, linux-crypto,
netdev, Herbert Xu, Tom Herbert, Eric Dumazet, David S. Miller,
stable
On Sun, Nov 24, 2013 at 3:42 PM, Richard Weinberger <richard@nod.at> wrote:
> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> added an internal flag MSG_SENDPAGE_NOTLAST.
> We have to ensure that MSG_MORE is also set if we set MSG_SENDPAGE_NOTLAST.
> Otherwise users that check against MSG_MORE will not see it.
>
> This fixes sendfile() on AF_ALG.
>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: <stable@vger.kernel.org> # 3.4.x
The offending commit also got backported to the 3.2 stable kernel, so
we need this fix there as well.
---
Shawn Landden
+1 360 389 3001 (SMS preferred)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST
2013-11-24 23:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
2013-11-25 0:03 ` Shawn Landden
@ 2013-11-25 1:25 ` Eric Dumazet
2013-11-25 1:40 ` Shawn Landden
` (3 more replies)
1 sibling, 4 replies; 17+ messages in thread
From: Eric Dumazet @ 2013-11-25 1:25 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, linux-fsdevel, viro, shawnlandden, linux-crypto,
netdev, herbert, Tom Herbert, David S. Miller, stable
On Mon, 2013-11-25 at 00:42 +0100, Richard Weinberger wrote:
> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> added an internal flag MSG_SENDPAGE_NOTLAST.
> We have to ensure that MSG_MORE is also set if we set MSG_SENDPAGE_NOTLAST.
> Otherwise users that check against MSG_MORE will not see it.
>
> This fixes sendfile() on AF_ALG.
>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: <stable@vger.kernel.org> # 3.4.x
> Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> fs/splice.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 3b7ee65..b93f1b8 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -701,7 +701,7 @@ static int pipe_to_sendpage(struct pipe_inode_info *pipe,
> more = (sd->flags & SPLICE_F_MORE) ? MSG_MORE : 0;
>
> if (sd->len < sd->total_len && pipe->nrbufs > 1)
> - more |= MSG_SENDPAGE_NOTLAST;
> + more |= MSG_SENDPAGE_NOTLAST | MSG_MORE;
>
> return file->f_op->sendpage(file, buf->page, buf->offset,
> sd->len, &pos, more);
I do not think this patch is right. It looks like a revert of a useful
patch for TCP zero copy. Given the time it took to discover this
regression, I bet tcp zero copy has more users than AF_ALG, by 5 or 6
order of magnitude ;)
Here we want to make the difference between the two flags, not merge
them.
If AF_ALG do not care of the difference, try instead :
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index ef5356cd280a..850246206b12 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
struct hash_ctx *ctx = ask->private;
int err;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
sg_init_table(ctx->sgl.sg, 1);
sg_set_page(ctx->sgl.sg, page, size, offset);
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST
2013-11-25 1:25 ` Eric Dumazet
@ 2013-11-25 1:40 ` Shawn Landden
2013-11-25 2:04 ` [PATCH] Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once) added an internal flag MSG_SENDPAGE_NOTLAST, similar to MSG_MORE Shawn Landden
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Shawn Landden @ 2013-11-25 1:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Richard Weinberger, Linux Kernel Mailing List, linux-fsdevel,
viro, linux-crypto, netdev, Herbert Xu, Tom Herbert,
David S. Miller, stable
On Sun, Nov 24, 2013 at 5:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2013-11-25 at 00:42 +0100, Richard Weinberger wrote:
>> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
>> added an internal flag MSG_SENDPAGE_NOTLAST.
>> We have to ensure that MSG_MORE is also set if we set MSG_SENDPAGE_NOTLAST.
>> Otherwise users that check against MSG_MORE will not see it.
>>
>> This fixes sendfile() on AF_ALG.
>>
>> Cc: Tom Herbert <therbert@google.com>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: <stable@vger.kernel.org> # 3.4.x
>> Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>> fs/splice.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/splice.c b/fs/splice.c
>> index 3b7ee65..b93f1b8 100644
>> --- a/fs/splice.c
>> +++ b/fs/splice.c
>> @@ -701,7 +701,7 @@ static int pipe_to_sendpage(struct pipe_inode_info *pipe,
>> more = (sd->flags & SPLICE_F_MORE) ? MSG_MORE : 0;
>>
>> if (sd->len < sd->total_len && pipe->nrbufs > 1)
>> - more |= MSG_SENDPAGE_NOTLAST;
>> + more |= MSG_SENDPAGE_NOTLAST | MSG_MORE;
>>
>> return file->f_op->sendpage(file, buf->page, buf->offset,
>> sd->len, &pos, more);
>
> I do not think this patch is right. It looks like a revert of a useful
> patch for TCP zero copy. Given the time it took to discover this
> regression, I bet tcp zero copy has more users than AF_ALG, by 5 or 6
> order of magnitude ;)
>
> Here we want to make the difference between the two flags, not merge
> them.
>
> If AF_ALG do not care of the difference, try instead :
>
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index ef5356cd280a..850246206b12 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
> struct hash_ctx *ctx = ask->private;
> int err;
>
> + if (flags & MSG_SENDPAGE_NOTLAST)
> + flags |= MSG_MORE;
> +
> lock_sock(sk);
> sg_init_table(ctx->sgl.sg, 1);
> sg_set_page(ctx->sgl.sg, page, size, offset);
>
>From my testing this works.
--
---
Shawn Landden
+1 360 389 3001 (SMS preferred)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once) added an internal flag MSG_SENDPAGE_NOTLAST, similar to MSG_MORE.
2013-11-25 1:25 ` Eric Dumazet
2013-11-25 1:40 ` Shawn Landden
@ 2013-11-25 2:04 ` Shawn Landden
2013-11-25 2:08 ` [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST Shawn Landden
2013-11-25 7:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
3 siblings, 0 replies; 17+ messages in thread
From: Shawn Landden @ 2013-11-25 2:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Mailing List, linux-fsdevel, viro, linux-crypto,
netdev, Herbert Xu, Tom Herbert, David S. Miller, stable,
Shawn Landden
algif_hash and algif_skcipher used MSG_MORE from tcp_sendpages()
and need to see the new flag as identical to MSG_MORE.
This fixes sendfile() on AF_ALG.
Cc: Tom Herbert <therbert@google.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: <stable@vger.kernel.org> # 3.4.x + 3.2.x
Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
Original-patch: Richard Weinberger <richard@nod.at>
Signed-off-by: Shawn Landden <shawn@churchofgit.com>
---
crypto/algif_hash.c | 3 +++
crypto/algif_skcipher.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index ef5356c..8502462 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
struct hash_ctx *ctx = ask->private;
int err;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
sg_init_table(ctx->sgl.sg, 1);
sg_set_page(ctx->sgl.sg, page, size, offset);
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 6a6dfc0..a19c027 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -378,6 +378,9 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
struct skcipher_sg_list *sgl;
int err = -EINVAL;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
if (!ctx->more && ctx->used)
goto unlock;
--
1.8.4.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-25 1:25 ` Eric Dumazet
2013-11-25 1:40 ` Shawn Landden
2013-11-25 2:04 ` [PATCH] Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once) added an internal flag MSG_SENDPAGE_NOTLAST, similar to MSG_MORE Shawn Landden
@ 2013-11-25 2:08 ` Shawn Landden
2013-11-25 4:26 ` Hannes Frederic Sowa
2013-11-25 7:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
3 siblings, 1 reply; 17+ messages in thread
From: Shawn Landden @ 2013-11-25 2:08 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Mailing List, linux-fsdevel, viro, linux-crypto,
netdev, Herbert Xu, Tom Herbert, David S. Miller, stable,
Shawn Landden
Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
added an internal flag MSG_SENDPAGE_NOTLAST, similar to
MSG_MORE.
algif_hash and algif_skcipher used MSG_MORE from tcp_sendpages()
and need to see the new flag as identical to MSG_MORE.
This fixes sendfile() on AF_ALG.
Cc: Tom Herbert <therbert@google.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: <stable@vger.kernel.org> # 3.4.x + 3.2.x
Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
Original-patch: Richard Weinberger <richard@nod.at>
Signed-off-by: Shawn Landden <shawn@churchofgit.com>
---
crypto/algif_hash.c | 3 +++
crypto/algif_skcipher.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index ef5356c..8502462 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
struct hash_ctx *ctx = ask->private;
int err;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
sg_init_table(ctx->sgl.sg, 1);
sg_set_page(ctx->sgl.sg, page, size, offset);
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 6a6dfc0..a19c027 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -378,6 +378,9 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
struct skcipher_sg_list *sgl;
int err = -EINVAL;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
if (!ctx->more && ctx->used)
goto unlock;
--
1.8.4.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-25 2:08 ` [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST Shawn Landden
@ 2013-11-25 4:26 ` Hannes Frederic Sowa
2013-11-25 6:36 ` Shawn Landden
0 siblings, 1 reply; 17+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-25 4:26 UTC (permalink / raw)
To: Shawn Landden
Cc: Eric Dumazet, Linux Kernel Mailing List, linux-fsdevel, viro,
linux-crypto, netdev, Herbert Xu, Tom Herbert, David S. Miller,
stable
On Sun, Nov 24, 2013 at 06:08:59PM -0800, Shawn Landden wrote:
> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> added an internal flag MSG_SENDPAGE_NOTLAST, similar to
> MSG_MORE.
>
> algif_hash and algif_skcipher used MSG_MORE from tcp_sendpages()
> and need to see the new flag as identical to MSG_MORE.
>
> This fixes sendfile() on AF_ALG.
Don't we need a similar fix for udp_sendpage?
Greetings,
Hannes
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-25 4:26 ` Hannes Frederic Sowa
@ 2013-11-25 6:36 ` Shawn Landden
2013-11-25 23:27 ` Richard Weinberger
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Shawn Landden @ 2013-11-25 6:36 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Eric Dumazet, linux-kernel, linux-fsdevel, viro, linux-crypto,
netdev, Herbert Xu, Tom Herbert, David S. Miller, stable,
Shawn Landden
Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
added an internal flag MSG_SENDPAGE_NOTLAST, similar to
MSG_MORE.
algif_hash, algif_skcipher, and udp used MSG_MORE from tcp_sendpages()
and need to see the new flag as identical to MSG_MORE.
This fixes sendfile() on AF_ALG.
v3: also fix udp
Cc: Tom Herbert <therbert@google.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: <stable@vger.kernel.org> # 3.4.x + 3.2.x
Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
Original-patch: Richard Weinberger <richard@nod.at>
Signed-off-by: Shawn Landden <shawn@churchofgit.com>
---
crypto/algif_hash.c | 3 +++
crypto/algif_skcipher.c | 3 +++
net/ipv4/udp.c | 3 +++
3 files changed, 9 insertions(+)
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index ef5356c..8502462 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
struct hash_ctx *ctx = ask->private;
int err;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
sg_init_table(ctx->sgl.sg, 1);
sg_set_page(ctx->sgl.sg, page, size, offset);
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 6a6dfc0..a19c027 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -378,6 +378,9 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
struct skcipher_sg_list *sgl;
int err = -EINVAL;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
lock_sock(sk);
if (!ctx->more && ctx->used)
goto unlock;
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 5944d7d..8bd04df 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1098,6 +1098,9 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
struct udp_sock *up = udp_sk(sk);
int ret;
+ if (flags & MSG_SENDPAGE_NOTLAST)
+ flags |= MSG_MORE;
+
if (!up->pending) {
struct msghdr msg = { .msg_flags = flags|MSG_MORE };
--
1.8.4.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST
2013-11-25 1:25 ` Eric Dumazet
` (2 preceding siblings ...)
2013-11-25 2:08 ` [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST Shawn Landden
@ 2013-11-25 7:42 ` Richard Weinberger
2013-11-26 0:02 ` Eric Dumazet
3 siblings, 1 reply; 17+ messages in thread
From: Richard Weinberger @ 2013-11-25 7:42 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel, linux-fsdevel, viro, shawnlandden, linux-crypto,
netdev, herbert, Tom Herbert, David S. Miller, stable
Am Sonntag, 24. November 2013, 17:25:06 schrieb Eric Dumazet:
> On Mon, 2013-11-25 at 00:42 +0100, Richard Weinberger wrote:
> > Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> > added an internal flag MSG_SENDPAGE_NOTLAST.
> > We have to ensure that MSG_MORE is also set if we set
> > MSG_SENDPAGE_NOTLAST.
> > Otherwise users that check against MSG_MORE will not see it.
> >
> > This fixes sendfile() on AF_ALG.
> >
> > Cc: Tom Herbert <therbert@google.com>
> > Cc: Eric Dumazet <eric.dumazet@gmail.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: <stable@vger.kernel.org> # 3.4.x
> > Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > ---
> >
> > fs/splice.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/splice.c b/fs/splice.c
> > index 3b7ee65..b93f1b8 100644
> > --- a/fs/splice.c
> > +++ b/fs/splice.c
> > @@ -701,7 +701,7 @@ static int pipe_to_sendpage(struct pipe_inode_info
> > *pipe,>
> > more = (sd->flags & SPLICE_F_MORE) ? MSG_MORE : 0;
> >
> > if (sd->len < sd->total_len && pipe->nrbufs > 1)
> >
> > - more |= MSG_SENDPAGE_NOTLAST;
> > + more |= MSG_SENDPAGE_NOTLAST | MSG_MORE;
> >
> > return file->f_op->sendpage(file, buf->page, buf->offset,
> >
> > sd->len, &pos, more);
>
> I do not think this patch is right. It looks like a revert of a useful
> patch for TCP zero copy. Given the time it took to discover this
> regression, I bet tcp zero copy has more users than AF_ALG, by 5 or 6
> order of magnitude ;)
Yeah, but AF_ALG broke. That's why I did the patch.
> Here we want to make the difference between the two flags, not merge
> them.
>
> If AF_ALG do not care of the difference, try instead :
>
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index ef5356cd280a..850246206b12 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct
> page *page, struct hash_ctx *ctx = ask->private;
> int err;
>
> + if (flags & MSG_SENDPAGE_NOTLAST)
> + flags |= MSG_MORE;
> +
In the commit message of your patch you wrote "For all sendpage() providers,
its a transparent change.". Why does AF_ALG need special handling?
If users have to care about MSG_SENDPAGE_NOTLAST it is no longer really an
internal flag.
Thanks,
//richard
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-25 6:36 ` Shawn Landden
@ 2013-11-25 23:27 ` Richard Weinberger
2013-11-29 5:47 ` Hannes Frederic Sowa
2013-11-29 21:33 ` David Miller
2 siblings, 0 replies; 17+ messages in thread
From: Richard Weinberger @ 2013-11-25 23:27 UTC (permalink / raw)
To: Shawn Landden
Cc: Hannes Frederic Sowa, Eric Dumazet, LKML, linux-fsdevel, Al Viro,
linux-crypto, netdev, Herbert Xu, Tom Herbert, David S. Miller,
stable
On Mon, Nov 25, 2013 at 7:36 AM, Shawn Landden <shawn@churchofgit.com> wrote:
> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> added an internal flag MSG_SENDPAGE_NOTLAST, similar to
> MSG_MORE.
>
> algif_hash, algif_skcipher, and udp used MSG_MORE from tcp_sendpages()
> and need to see the new flag as identical to MSG_MORE.
>
> This fixes sendfile() on AF_ALG.
>
> v3: also fix udp
>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: <stable@vger.kernel.org> # 3.4.x + 3.2.x
> Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
> Original-patch: Richard Weinberger <richard@nod.at>
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
May I ask why you took over the my patch without even CC'in me nor
replying to the original
thread "[PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we
set MSG_SENDPAGE_NOTLAST"?
You are acting very rude.
The discussion at the original thread is not done.
Does skcipher_sendpage() really also need fixing? or UDP?
I didn't send another patch because I'm waiting for Eric's answer first.
Thanks,
//richard
> ---
> crypto/algif_hash.c | 3 +++
> crypto/algif_skcipher.c | 3 +++
> net/ipv4/udp.c | 3 +++
> 3 files changed, 9 insertions(+)
>
> diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
> index ef5356c..8502462 100644
> --- a/crypto/algif_hash.c
> +++ b/crypto/algif_hash.c
> @@ -114,6 +114,9 @@ static ssize_t hash_sendpage(struct socket *sock, struct page *page,
> struct hash_ctx *ctx = ask->private;
> int err;
>
> + if (flags & MSG_SENDPAGE_NOTLAST)
> + flags |= MSG_MORE;
> +
> lock_sock(sk);
> sg_init_table(ctx->sgl.sg, 1);
> sg_set_page(ctx->sgl.sg, page, size, offset);
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 6a6dfc0..a19c027 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -378,6 +378,9 @@ static ssize_t skcipher_sendpage(struct socket *sock, struct page *page,
> struct skcipher_sg_list *sgl;
> int err = -EINVAL;
>
> + if (flags & MSG_SENDPAGE_NOTLAST)
> + flags |= MSG_MORE;
> +
> lock_sock(sk);
> if (!ctx->more && ctx->used)
> goto unlock;
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 5944d7d..8bd04df 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1098,6 +1098,9 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
> struct udp_sock *up = udp_sk(sk);
> int ret;
>
> + if (flags & MSG_SENDPAGE_NOTLAST)
> + flags |= MSG_MORE;
> +
> if (!up->pending) {
> struct msghdr msg = { .msg_flags = flags|MSG_MORE };
>
> --
> 1.8.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST
2013-11-25 7:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
@ 2013-11-26 0:02 ` Eric Dumazet
0 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2013-11-26 0:02 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-kernel, linux-fsdevel, viro, shawnlandden, linux-crypto,
netdev, herbert, Tom Herbert, David S. Miller, stable
On Mon, 2013-11-25 at 08:42 +0100, Richard Weinberger wrote:
> In the commit message of your patch you wrote "For all sendpage() providers,
> its a transparent change.". Why does AF_ALG need special handling?
> If users have to care about MSG_SENDPAGE_NOTLAST it is no longer really an
> internal flag.
MSG_SENDPAGE_NOTLAST is an internal (kernel) flag.
Fact that I missed some MSG_MORE 'users' in the kernel was an oversight.
I am not saying your patch is not needed, I am only saying it reverts
a useful TCP optimization, and we can do better, dont we ?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-25 6:36 ` Shawn Landden
2013-11-25 23:27 ` Richard Weinberger
@ 2013-11-29 5:47 ` Hannes Frederic Sowa
2013-11-29 8:50 ` Richard Weinberger
2013-11-29 21:33 ` David Miller
2 siblings, 1 reply; 17+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-29 5:47 UTC (permalink / raw)
To: Shawn Landden
Cc: Eric Dumazet, linux-kernel, linux-fsdevel, viro, linux-crypto,
netdev, Herbert Xu, Tom Herbert, David S. Miller, stable,
richard.weinberger
On Sun, Nov 24, 2013 at 10:36:28PM -0800, Shawn Landden wrote:
> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> added an internal flag MSG_SENDPAGE_NOTLAST, similar to
> MSG_MORE.
>
> algif_hash, algif_skcipher, and udp used MSG_MORE from tcp_sendpages()
> and need to see the new flag as identical to MSG_MORE.
>
> This fixes sendfile() on AF_ALG.
>
> v3: also fix udp
The UDP bits look fine to me.
Greetings,
Hannes
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-29 5:47 ` Hannes Frederic Sowa
@ 2013-11-29 8:50 ` Richard Weinberger
0 siblings, 0 replies; 17+ messages in thread
From: Richard Weinberger @ 2013-11-29 8:50 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Shawn Landden, Eric Dumazet, linux-kernel, linux-fsdevel, viro,
linux-crypto, netdev, Herbert Xu, Tom Herbert, David S. Miller,
stable
Shawn,
Am Freitag, 29. November 2013, 06:47:04 schrieb Hannes Frederic Sowa:
> On Sun, Nov 24, 2013 at 10:36:28PM -0800, Shawn Landden wrote:
> > Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> > added an internal flag MSG_SENDPAGE_NOTLAST, similar to
> > MSG_MORE.
> >
> > algif_hash, algif_skcipher, and udp used MSG_MORE from tcp_sendpages()
> > and need to see the new flag as identical to MSG_MORE.
> >
> > This fixes sendfile() on AF_ALG.
> >
> > v3: also fix udp
>
> The UDP bits look fine to me.
please don't forget to resend the patch (only the UDP fix).
I sent the AF_ALG part already.
Thanks,
//richard
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST
2013-11-25 6:36 ` Shawn Landden
2013-11-25 23:27 ` Richard Weinberger
2013-11-29 5:47 ` Hannes Frederic Sowa
@ 2013-11-29 21:33 ` David Miller
2 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2013-11-29 21:33 UTC (permalink / raw)
To: shawn
Cc: hannes, eric.dumazet, linux-kernel, linux-fsdevel, viro,
linux-crypto, netdev, herbert, therbert, stable
From: Shawn Landden <shawn@churchofgit.com>
Date: Sun, 24 Nov 2013 22:36:28 -0800
> Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once)
> added an internal flag MSG_SENDPAGE_NOTLAST, similar to
> MSG_MORE.
>
> algif_hash, algif_skcipher, and udp used MSG_MORE from tcp_sendpages()
> and need to see the new flag as identical to MSG_MORE.
>
> This fixes sendfile() on AF_ALG.
>
> v3: also fix udp
>
> Cc: Tom Herbert <therbert@google.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: <stable@vger.kernel.org> # 3.4.x + 3.2.x
> Reported-and-tested-by: Shawn Landden <shawnlandden@gmail.com>
> Original-patch: Richard Weinberger <richard@nod.at>
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
Applied and queued up for -stable, thanks everyone.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-11-29 21:33 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-24 17:21 AF_ALG buggy with sendfile Shawn Landden
2013-11-24 22:00 ` Shawn Landden
2013-11-24 22:04 ` Shawn Landden
2013-11-24 23:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
2013-11-25 0:03 ` Shawn Landden
2013-11-25 1:25 ` Eric Dumazet
2013-11-25 1:40 ` Shawn Landden
2013-11-25 2:04 ` [PATCH] Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once) added an internal flag MSG_SENDPAGE_NOTLAST, similar to MSG_MORE Shawn Landden
2013-11-25 2:08 ` [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST Shawn Landden
2013-11-25 4:26 ` Hannes Frederic Sowa
2013-11-25 6:36 ` Shawn Landden
2013-11-25 23:27 ` Richard Weinberger
2013-11-29 5:47 ` Hannes Frederic Sowa
2013-11-29 8:50 ` Richard Weinberger
2013-11-29 21:33 ` David Miller
2013-11-25 7:42 ` [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST Richard Weinberger
2013-11-26 0:02 ` Eric Dumazet
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).