| Message ID | 20230620102856.56074-3-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/tls: fixes for NVMe-over-TLS | expand |
2023-06-20, 12:28:54 +0200, Hannes Reinecke wrote: > tls_push_data() MSG_MORE / MSG_SENDPAGE_NOTLAST, but bails > out on MSG_EOR. > But seeing that MSG_EOR is basically the opposite of > MSG_MORE / MSG_SENDPAGE_NOTLAST this patch adds handling > MSG_EOR by treating it as the absence of MSG_MORE. > Consequently we should return an error when both are set. > > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > net/tls/tls_device.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c > index b82770f68807..ebefd148ecf5 100644 > --- a/net/tls/tls_device.c > +++ b/net/tls/tls_device.c > @@ -440,11 +440,6 @@ static int tls_push_data(struct sock *sk, > int copy, rc = 0; > long timeo; > > - if (flags & > - ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | > - MSG_SPLICE_PAGES)) > - return -EOPNOTSUPP; > - > if (unlikely(sk->sk_err)) > return -sk->sk_err; > > @@ -536,6 +531,10 @@ static int tls_push_data(struct sock *sk, > more = true; > break; > } > + if (flags & MSG_EOR) { > + more = false; > + break; Why the break here? We don't want to close and push the record in that case? (the "if (done || ...)" block just below) > + } > > done = true; > } Thanks,
On 6/20/23 19:12, Sabrina Dubroca wrote: > 2023-06-20, 12:28:54 +0200, Hannes Reinecke wrote: >> tls_push_data() MSG_MORE / MSG_SENDPAGE_NOTLAST, but bails >> out on MSG_EOR. >> But seeing that MSG_EOR is basically the opposite of >> MSG_MORE / MSG_SENDPAGE_NOTLAST this patch adds handling >> MSG_EOR by treating it as the absence of MSG_MORE. >> Consequently we should return an error when both are set. >> >> Cc: Jakub Kicinski <kuba@kernel.org> >> Cc: netdev@vger.kernel.org >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> net/tls/tls_device.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c >> index b82770f68807..ebefd148ecf5 100644 >> --- a/net/tls/tls_device.c >> +++ b/net/tls/tls_device.c >> @@ -440,11 +440,6 @@ static int tls_push_data(struct sock *sk, >> int copy, rc = 0; >> long timeo; >> >> - if (flags & >> - ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | >> - MSG_SPLICE_PAGES)) >> - return -EOPNOTSUPP; >> - >> if (unlikely(sk->sk_err)) >> return -sk->sk_err; >> >> @@ -536,6 +531,10 @@ static int tls_push_data(struct sock *sk, >> more = true; >> break; >> } >> + if (flags & MSG_EOR) { >> + more = false; >> + break; > > Why the break here? We don't want to close and push the record in that > case? (the "if (done || ...)" block just below) > Ah, yes, you are correct. Will be fixing it. Cheers, Hannes
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index b82770f68807..ebefd148ecf5 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -440,11 +440,6 @@ static int tls_push_data(struct sock *sk, int copy, rc = 0; long timeo; - if (flags & - ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | - MSG_SPLICE_PAGES)) - return -EOPNOTSUPP; - if (unlikely(sk->sk_err)) return -sk->sk_err; @@ -536,6 +531,10 @@ static int tls_push_data(struct sock *sk, more = true; break; } + if (flags & MSG_EOR) { + more = false; + break; + } done = true; } @@ -582,6 +581,14 @@ int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) if (!tls_ctx->zerocopy_sendfile) msg->msg_flags &= ~MSG_SPLICE_PAGES; + if (msg->msg_flags & + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | + MSG_SPLICE_PAGES | MSG_EOR)) + return -EOPNOTSUPP; + + if ((msg->msg_flags & (MSG_MORE | MSG_EOR)) == (MSG_MORE | MSG_EOR)) + return -EOPNOTSUPP; + mutex_lock(&tls_ctx->tx_lock); lock_sock(sk); @@ -627,9 +634,17 @@ int tls_device_sendpage(struct sock *sk, struct page *page, struct bio_vec bvec; struct msghdr msg = { .msg_flags = flags | MSG_SPLICE_PAGES, }; + if (flags & + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | + MSG_SPLICE_PAGES | MSG_EOR)) + return -EOPNOTSUPP; + if (flags & MSG_SENDPAGE_NOTLAST) msg.msg_flags |= MSG_MORE; + if ((msg.msg_flags & (MSG_MORE | MSG_EOR)) == (MSG_MORE | MSG_EOR)) + return -EINVAL; + if (flags & MSG_OOB) return -EOPNOTSUPP;
tls_push_data() MSG_MORE / MSG_SENDPAGE_NOTLAST, but bails out on MSG_EOR. But seeing that MSG_EOR is basically the opposite of MSG_MORE / MSG_SENDPAGE_NOTLAST this patch adds handling MSG_EOR by treating it as the absence of MSG_MORE. Consequently we should return an error when both are set. Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org Signed-off-by: Hannes Reinecke <hare@suse.de> --- net/tls/tls_device.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)