| Message ID | 20230612143833.70805-3-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/tls: fixes for NVMe-over-TLS | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 6/12/23 17:38, 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 | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c > index a7cc4f9faac2..0024febd40de 100644 > --- a/net/tls/tls_device.c > +++ b/net/tls/tls_device.c > @@ -448,10 +448,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)) > - return -EOPNOTSUPP; > - > if (unlikely(sk->sk_err)) > return -sk->sk_err; > > @@ -529,6 +525,10 @@ static int tls_push_data(struct sock *sk, > more = true; > break; > } > + if (flags & MSG_EOR) { > + more = false; > + break; > + } > > done = true; > } > @@ -573,6 +573,14 @@ int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > union tls_iter_offset iter; > int rc; > > + if (msg->msg_flags & > + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_EOR)) > + return -EOPNOTSUPP; > + > + if ((msg->msg_flags & MSG_MORE) && > + (msg->msg_flags & MSG_EOR)) > + return -EOPNOTSUPP; EINVAL is more appropriate I think... > + > mutex_lock(&tls_ctx->tx_lock); > lock_sock(sk); > > @@ -601,9 +609,17 @@ int tls_device_sendpage(struct sock *sk, struct page *page, > struct kvec iov; > int rc; > > + if (flags & > + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | MSG_EOR)) > + return -EOPNOTSUPP; > + > if (flags & MSG_SENDPAGE_NOTLAST) > flags |= MSG_MORE; > > + if ((flags & MSG_MORE) && > + (flags & MSG_EOR)) > + return -EOPNOTSUPP; EINVAL?
On 6/13/23 09:58, Sagi Grimberg wrote: > > > On 6/12/23 17:38, 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 | 24 ++++++++++++++++++++---- >> 1 file changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c >> index a7cc4f9faac2..0024febd40de 100644 >> --- a/net/tls/tls_device.c >> +++ b/net/tls/tls_device.c >> @@ -448,10 +448,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)) >> - return -EOPNOTSUPP; >> - >> if (unlikely(sk->sk_err)) >> return -sk->sk_err; >> @@ -529,6 +525,10 @@ static int tls_push_data(struct sock *sk, >> more = true; >> break; >> } >> + if (flags & MSG_EOR) { >> + more = false; >> + break; >> + } >> done = true; >> } >> @@ -573,6 +573,14 @@ int tls_device_sendmsg(struct sock *sk, struct >> msghdr *msg, size_t size) >> union tls_iter_offset iter; >> int rc; >> + if (msg->msg_flags & >> + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_EOR)) >> + return -EOPNOTSUPP; >> + >> + if ((msg->msg_flags & MSG_MORE) && >> + (msg->msg_flags & MSG_EOR)) >> + return -EOPNOTSUPP; > > EINVAL is more appropriate I think... > Guess what, that's what I did initially. But then when returning EINVAL we would arguably introduce a regression (as suddenly we'll be returning a different error code as previously). So with this patch we're backwards compatible. But that's really a quesion for Jakub: what's more appropriate here? Return a new error code (which describes the situation better) or stick with the original one (and retain compability)? Cheers, Hannes
On Tue, 13 Jun 2023 10:11:01 +0200 Hannes Reinecke wrote: > >> + if ((msg->msg_flags & MSG_MORE) && > >> + (msg->msg_flags & MSG_EOR)) > >> + return -EOPNOTSUPP; > > > > EINVAL is more appropriate I think... > > > Guess what, that's what I did initially. > But then when returning EINVAL we would arguably introduce a regression > (as suddenly we'll be returning a different error code as previously). > So with this patch we're backwards compatible. > > But that's really a quesion for Jakub: what's more appropriate here? > Return a new error code (which describes the situation better) or stick > with the original one (and retain compability)? EINVAL sounds better, EOPNOTSUPP means not implemented yet, once the thing is implemented it's natural that we'll start returning more precise error codes. BTW you need to respin on top of net-next, David's multi-page sendpage has rejigged this code quite a bit.
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index a7cc4f9faac2..0024febd40de 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -448,10 +448,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)) - return -EOPNOTSUPP; - if (unlikely(sk->sk_err)) return -sk->sk_err; @@ -529,6 +525,10 @@ static int tls_push_data(struct sock *sk, more = true; break; } + if (flags & MSG_EOR) { + more = false; + break; + } done = true; } @@ -573,6 +573,14 @@ int tls_device_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) union tls_iter_offset iter; int rc; + if (msg->msg_flags & + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_EOR)) + return -EOPNOTSUPP; + + if ((msg->msg_flags & MSG_MORE) && + (msg->msg_flags & MSG_EOR)) + return -EOPNOTSUPP; + mutex_lock(&tls_ctx->tx_lock); lock_sock(sk); @@ -601,9 +609,17 @@ int tls_device_sendpage(struct sock *sk, struct page *page, struct kvec iov; int rc; + if (flags & + ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST | MSG_EOR)) + return -EOPNOTSUPP; + if (flags & MSG_SENDPAGE_NOTLAST) flags |= MSG_MORE; + if ((flags & MSG_MORE) && + (flags & MSG_EOR)) + return -EOPNOTSUPP; + mutex_lock(&tls_ctx->tx_lock); lock_sock(sk);
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 | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)