On Tue, Jun 2, 2020 at 3:19 PM Jakub Kicinski <kuba@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
> 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);
> > +}
>
