[RFC PATCH net 1/1] net/tls(TLS_SW): Add selftest for 'chunked' sendfile test

[RFC PATCH net 1/1] net/tls(TLS_SW): Add selftest for 'chunked' sendfile test

[Pooja Trivedi]

This selftest tests for cases where sendfile's 'count'
parameter is provided with a size greater than the intended
file size.

Motivation: When sendfile is provided with 'count' parameter
value that is greater than the size of the file, kTLS example
fails to send the file correctly. Last chunk of the file is
not sent, and the data integrity is compromised.
The reason is that the last chunk has MSG_MORE flag set
because of which it gets added to pending records, but is
not pushed.
Note that if user space were to send SSL_shutdown control
message, pending records would get flushed and the issue
would not happen. So a shutdown control message following
sendfile can mask the issue.

Signed-off-by: Pooja Trivedi <pooja.trivedi@stackpath.com>
---
 tools/testing/selftests/net/tls.c | 58 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 0ea44d9..f0455e6 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -198,6 +198,64 @@
 	EXPECT_EQ(recv(self->cfd, buf, st.st_size, MSG_WAITALL), st.st_size);
 }
 
+static void chunked_sendfile(struct __test_metadata *_metadata,
+			     struct _test_data_tls *self,
+			     uint16_t chunk_size,
+			     uint16_t extra_payload_size)
+{
+	char buf[TLS_PAYLOAD_MAX_LEN];
+	uint16_t test_payload_size;
+	int size = 0;
+	int ret;
+	char tmpfile[] = ".TMP_ktls";
+	int fd = open(tmpfile, O_RDWR | O_CREAT | O_TRUNC, 0644);
+	off_t offset = 0;
+
+	ASSERT_GE(fd, 0);
+	EXPECT_GE(chunk_size, 1);
+	test_payload_size = chunk_size + extra_payload_size;
+	ASSERT_GE(TLS_PAYLOAD_MAX_LEN, test_payload_size);
+	memset(buf, 1, test_payload_size);
+	size = write(fd, buf, test_payload_size);
+	EXPECT_EQ(size, test_payload_size);
+	fsync(fd);
+
+	while (size > 0) {
+		ret = sendfile(self->fd, fd, &offset, chunk_size);
+		EXPECT_GE(ret, 0);
+		size -= ret;
+	}
+
+	EXPECT_EQ(recv(self->cfd, buf, test_payload_size, MSG_WAITALL),
+		  test_payload_size);
+
+	close(fd);
+	unlink(tmpfile);
+}
+
+TEST_F(tls, multi_chunk_sendfile)
+{
+	chunked_sendfile(_metadata, self, 4096, 4096);
+	chunked_sendfile(_metadata, self, 4096, 0);
+	chunked_sendfile(_metadata, self, 4096, 1);
+	chunked_sendfile(_metadata, self, 4096, 2048);
+	chunked_sendfile(_metadata, self, 8192, 2048);
+	chunked_sendfile(_metadata, self, 4096, 8192);
+	chunked_sendfile(_metadata, self, 8192, 4096);
+	chunked_sendfile(_metadata, self, 12288, 1024);
+	chunked_sendfile(_metadata, self, 12288, 2000);
+	chunked_sendfile(_metadata, self, 15360, 100);
+	chunked_sendfile(_metadata, self, 15360, 300);
+	chunked_sendfile(_metadata, self, 1, 4096);
+	chunked_sendfile(_metadata, self, 2048, 4096);
+	chunked_sendfile(_metadata, self, 2048, 8192);
+	chunked_sendfile(_metadata, self, 4096, 8192);
+	chunked_sendfile(_metadata, self, 1024, 12288);
+	chunked_sendfile(_metadata, self, 2000, 12288);
+	chunked_sendfile(_metadata, self, 100, 15360);
+	chunked_sendfile(_metadata, self, 300, 15360);
+}
+
 TEST_F(tls, recv_max)
 {
 	unsigned int send_len = TLS_PAYLOAD_MAX_LEN;
-- 
1.8.3.1

Re: [RFC PATCH net 1/1] net/tls(TLS_SW): Add selftest for 'chunked' sendfile test

[Jakub Kicinski]

On Tue,  2 Jun 2020 14:56:25 +0000 Pooja Trivedi wrote:
> This selftest tests for cases where sendfile's 'count'
> parameter is provided with a size greater than the intended
> file size.
> 
> Motivation: When sendfile is provided with 'count' parameter
> value that is greater than the size of the file, kTLS example
> fails to send the file correctly. Last chunk of the file is
> not sent, and the data integrity is compromised.
> The reason is that the last chunk has MSG_MORE flag set
> because of which it gets added to pending records, but is
> not pushed.
> Note that if user space were to send SSL_shutdown control
> message, pending records would get flushed and the issue
> would not happen. So a shutdown control message following
> sendfile can mask the issue.
> 
> Signed-off-by: Pooja Trivedi <pooja.trivedi@stackpath.com>

Looks good, thanks. Did you submit the change to splice officially?
We'd need to get an Ack from VFS folks on it (Al Viro, probably?)
or even merge it via the vfs tree.

Minor nits below.

> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index 0ea44d9..f0455e6 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
> @@ -198,6 +198,64 @@
>  	EXPECT_EQ(recv(self->cfd, buf, st.st_size, MSG_WAITALL), st.st_size);
>  }
>  
> +static void chunked_sendfile(struct __test_metadata *_metadata,
> +			     struct _test_data_tls *self,
> +			     uint16_t chunk_size,
> +			     uint16_t extra_payload_size)
> +{
> +	char buf[TLS_PAYLOAD_MAX_LEN];
> +	uint16_t test_payload_size;
> +	int size = 0;
> +	int ret;
> +	char tmpfile[] = ".TMP_ktls";

Could we place the file in /tmp and use mktemp()? I sometimes run the
selftests from a read-only NFS mount, and trying to create a file in
current dir breaks that.

> +	int fd = open(tmpfile, O_RDWR | O_CREAT | O_TRUNC, 0644);

We can unlink right after we open. The file won't get removed as long
as we have a reference to it, and we minimize the risk of leaving it
behind.

> +	off_t offset = 0;
> +
> +	ASSERT_GE(fd, 0);
> +	EXPECT_GE(chunk_size, 1);
> +	test_payload_size = chunk_size + extra_payload_size;
> +	ASSERT_GE(TLS_PAYLOAD_MAX_LEN, test_payload_size);
> +	memset(buf, 1, test_payload_size);
> +	size = write(fd, buf, test_payload_size);
> +	EXPECT_EQ(size, test_payload_size);
> +	fsync(fd);
> +
> +	while (size > 0) {
> +		ret = sendfile(self->fd, fd, &offset, chunk_size);
> +		EXPECT_GE(ret, 0);
> +		size -= ret;
> +	}
> +
> +	EXPECT_EQ(recv(self->cfd, buf, test_payload_size, MSG_WAITALL),
> +		  test_payload_size);
> +
> +	close(fd);
> +	unlink(tmpfile);
> +}

Re: [RFC PATCH net 1/1] net/tls(TLS_SW): Add selftest for 'chunked' sendfile test

[Pooja Trivedi]

On Tue, Jun 2, 2020 at 3:19 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  2 Jun 2020 14:56:25 +0000 Pooja Trivedi wrote:
> > This selftest tests for cases where sendfile's 'count'
> > parameter is provided with a size greater than the intended
> > file size.
> >
> > Motivation: When sendfile is provided with 'count' parameter
> > value that is greater than the size of the file, kTLS example
> > fails to send the file correctly. Last chunk of the file is
> > not sent, and the data integrity is compromised.
> > The reason is that the last chunk has MSG_MORE flag set
> > because of which it gets added to pending records, but is
> > not pushed.
> > Note that if user space were to send SSL_shutdown control
> > message, pending records would get flushed and the issue
> > would not happen. So a shutdown control message following
> > sendfile can mask the issue.
> >
> > Signed-off-by: Pooja Trivedi <pooja.trivedi@stackpath.com>
>
> Looks good, thanks. Did you submit the change to splice officially?
> We'd need to get an Ack from VFS folks on it (Al Viro, probably?)
> or even merge it via the vfs tree.
>

No, I did not submit the change to splice yet. I can do that next.
I wanted to first run this through here and hear thoughts/suggestions.

> Minor nits below.
>

Will change and resubmit the selftest. Thanks.

> > diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> > index 0ea44d9..f0455e6 100644
> > --- a/tools/testing/selftests/net/tls.c
> > +++ b/tools/testing/selftests/net/tls.c
> > @@ -198,6 +198,64 @@
> >       EXPECT_EQ(recv(self->cfd, buf, st.st_size, MSG_WAITALL), st.st_size);
> >  }
> >
> > +static void chunked_sendfile(struct __test_metadata *_metadata,
> > +                          struct _test_data_tls *self,
> > +                          uint16_t chunk_size,
> > +                          uint16_t extra_payload_size)
> > +{
> > +     char buf[TLS_PAYLOAD_MAX_LEN];
> > +     uint16_t test_payload_size;
> > +     int size = 0;
> > +     int ret;
> > +     char tmpfile[] = ".TMP_ktls";
>
> Could we place the file in /tmp and use mktemp()? I sometimes run the
> selftests from a read-only NFS mount, and trying to create a file in
> current dir breaks that.
>
> > +     int fd = open(tmpfile, O_RDWR | O_CREAT | O_TRUNC, 0644);
>
> We can unlink right after we open. The file won't get removed as long
> as we have a reference to it, and we minimize the risk of leaving it
> behind.
>
> > +     off_t offset = 0;
> > +
> > +     ASSERT_GE(fd, 0);
> > +     EXPECT_GE(chunk_size, 1);
> > +     test_payload_size = chunk_size + extra_payload_size;
> > +     ASSERT_GE(TLS_PAYLOAD_MAX_LEN, test_payload_size);
> > +     memset(buf, 1, test_payload_size);
> > +     size = write(fd, buf, test_payload_size);
> > +     EXPECT_EQ(size, test_payload_size);
> > +     fsync(fd);
> > +
> > +     while (size > 0) {
> > +             ret = sendfile(self->fd, fd, &offset, chunk_size);
> > +             EXPECT_GE(ret, 0);
> > +             size -= ret;
> > +     }
> > +
> > +     EXPECT_EQ(recv(self->cfd, buf, test_payload_size, MSG_WAITALL),
> > +               test_payload_size);
> > +
> > +     close(fd);
> > +     unlink(tmpfile);
> > +}
>