AF_ALG buggy with sendfile

AF_ALG buggy with sendfile

[Shawn Landden]

[-- 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;
}

Re: AF_ALG buggy with sendfile

[Shawn Landden]

[-- 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;
}

Re: AF_ALG buggy with sendfile

[Shawn Landden]

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)

[PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

[Richard Weinberger]

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

Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

[Shawn Landden]

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)

Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

[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 ;)

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);

Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

[Shawn Landden]

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)

[PATCH] Commit 35f9c09fe (tcp: tcp_sendpages() should call tcp_push() once) added an internal flag MSG_SENDPAGE_NOTLAST, similar to MSG_MORE.

[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

[PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[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

Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[Hannes Frederic Sowa]

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

[PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[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

Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[Richard Weinberger]

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

Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[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.

Greetings,

  Hannes

Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[Richard Weinberger]

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

Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[David Miller]

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.

Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

[Richard Weinberger]

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

Re: [PATCH] pipe_to_sendpage: Ensure that MSG_MORE is set if we set MSG_SENDPAGE_NOTLAST

[Eric Dumazet]

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 ?

Re: [PATCH] update consumers of MSG_MORE to recognize MSG_SENDPAGE_NOTLAST

[Shawn Landden]

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.
Not CCing you was an oversight.

>The discussion at the original thread is not done.
>Does skcipher_sendpage() really also need fixing? or UDP?

UDP needs it or it will send out packets mid-sendfile. skcipher needs
it or it will produce incorrect
output like hash.
>I didn't send another patch because I'm waiting for Eric's answer first.
Eric forgot to update consumers internal consumers of MSG_MORE when he created
this distinction in sendpage. That became clear when he first
responded. Changing this where the consumers are makes sense. I just
want this bug fixed, feel free to send your next version.

>Thanks,
>//richard

-Shawn