| Message ID | 20230602150752.1306532-4-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | splice, net: Rewrite splice-to-socket, fix SPLICE_F_MORE and handle MSG_SPLICE_PAGES in AF_TLS | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
+ dan Carpenter On Fri, Jun 02, 2023 at 04:07:44PM +0100, David Howells wrote: > Allow userspace to end a TLS record without supplying any data by calling > send()/sendto()/sendmsg() with no data and no MSG_MORE flag. This can be > used to flush a previous send/splice that had MSG_MORE or SPLICE_F_MORE set > or a sendfile() that was incomplete. > > Without this, a zero-length send to tls-sw is just ignored. I think > tls-device will do the right thing without modification. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Chuck Lever <chuck.lever@oracle.com> > cc: Boris Pismenny <borisp@nvidia.com> > cc: John Fastabend <john.fastabend@gmail.com> > cc: Jakub Kicinski <kuba@kernel.org> > cc: Eric Dumazet <edumazet@google.com> > cc: "David S. Miller" <davem@davemloft.net> > cc: Paolo Abeni <pabeni@redhat.com> > cc: Jens Axboe <axboe@kernel.dk> > cc: Matthew Wilcox <willy@infradead.org> > cc: netdev@vger.kernel.org > --- > net/tls/tls_sw.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index cac1adc968e8..6aa6d17888f5 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -945,7 +945,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > struct tls_rec *rec; > int required_size; > int num_async = 0; > - bool full_record; > + bool full_record = false; > int record_room; > int num_zc = 0; > int orig_size; > @@ -971,6 +971,9 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > } > } > > + if (!msg_data_left(msg) && eor) > + goto just_flush; > + Hi David, the flow of this function is not entirely simple, so it is not easy for me to manually verify this. But in combination gcc-12 -Wmaybe-uninitialized and Smatch report that the following may be used uninitialised as a result of this change: * msg_pl * orig_size * msg_en * required_size * try_to_copy > while (msg_data_left(msg)) { > if (sk->sk_err) { > ret = -sk->sk_err; > @@ -1082,6 +1085,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > */ > tls_ctx->pending_open_record_frags = true; > copied += try_to_copy; > +just_flush: > if (full_record || eor) { > ret = bpf_exec_tx_verdict(msg_pl, sk, full_record, > record_type, &copied, > >
On Fri, Jun 02, 2023 at 08:27:56PM +0200, Simon Horman wrote: > + dan Carpenter > > On Fri, Jun 02, 2023 at 04:07:44PM +0100, David Howells wrote: > > Allow userspace to end a TLS record without supplying any data by calling > > send()/sendto()/sendmsg() with no data and no MSG_MORE flag. This can be > > used to flush a previous send/splice that had MSG_MORE or SPLICE_F_MORE set > > or a sendfile() that was incomplete. > > > > Without this, a zero-length send to tls-sw is just ignored. I think > > tls-device will do the right thing without modification. > > > > Signed-off-by: David Howells <dhowells@redhat.com> > > cc: Chuck Lever <chuck.lever@oracle.com> > > cc: Boris Pismenny <borisp@nvidia.com> > > cc: John Fastabend <john.fastabend@gmail.com> > > cc: Jakub Kicinski <kuba@kernel.org> > > cc: Eric Dumazet <edumazet@google.com> > > cc: "David S. Miller" <davem@davemloft.net> > > cc: Paolo Abeni <pabeni@redhat.com> > > cc: Jens Axboe <axboe@kernel.dk> > > cc: Matthew Wilcox <willy@infradead.org> > > cc: netdev@vger.kernel.org > > --- > > net/tls/tls_sw.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > > index cac1adc968e8..6aa6d17888f5 100644 > > --- a/net/tls/tls_sw.c > > +++ b/net/tls/tls_sw.c > > @@ -945,7 +945,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > > struct tls_rec *rec; > > int required_size; > > int num_async = 0; > > - bool full_record; > > + bool full_record = false; > > int record_room; > > int num_zc = 0; > > int orig_size; > > @@ -971,6 +971,9 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > > } > > } > > > > + if (!msg_data_left(msg) && eor) > > + goto just_flush; > > + > > Hi David, > > the flow of this function is not entirely simple, so it is not easy for me > to manually verify this. But in combination gcc-12 -Wmaybe-uninitialized > and Smatch report that the following may be used uninitialised as a result > of this change: > > * msg_pl This warning seems correct to me. > * orig_size This warning assumes we hit the first warning and then hit the goto wait_for_memory; > * msg_en I don't get this warning on my system but it's the same thing. Hit the first warning then the goto wait_for_memory. > * required_size Same. > * try_to_copy I don't really understand this warning and I can't reproduce it. Strange. regards, dan carpenter
On Fri, Jun 02, 2023 at 10:00:45PM +0300, Dan Carpenter wrote: > On Fri, Jun 02, 2023 at 08:27:56PM +0200, Simon Horman wrote: > > + dan Carpenter > > > > On Fri, Jun 02, 2023 at 04:07:44PM +0100, David Howells wrote: > > > Allow userspace to end a TLS record without supplying any data by calling > > > send()/sendto()/sendmsg() with no data and no MSG_MORE flag. This can be > > > used to flush a previous send/splice that had MSG_MORE or SPLICE_F_MORE set > > > or a sendfile() that was incomplete. > > > > > > Without this, a zero-length send to tls-sw is just ignored. I think > > > tls-device will do the right thing without modification. > > > > > > Signed-off-by: David Howells <dhowells@redhat.com> > > > cc: Chuck Lever <chuck.lever@oracle.com> > > > cc: Boris Pismenny <borisp@nvidia.com> > > > cc: John Fastabend <john.fastabend@gmail.com> > > > cc: Jakub Kicinski <kuba@kernel.org> > > > cc: Eric Dumazet <edumazet@google.com> > > > cc: "David S. Miller" <davem@davemloft.net> > > > cc: Paolo Abeni <pabeni@redhat.com> > > > cc: Jens Axboe <axboe@kernel.dk> > > > cc: Matthew Wilcox <willy@infradead.org> > > > cc: netdev@vger.kernel.org > > > --- > > > net/tls/tls_sw.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > > > index cac1adc968e8..6aa6d17888f5 100644 > > > --- a/net/tls/tls_sw.c > > > +++ b/net/tls/tls_sw.c > > > @@ -945,7 +945,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > > > struct tls_rec *rec; > > > int required_size; > > > int num_async = 0; > > > - bool full_record; > > > + bool full_record = false; > > > int record_room; > > > int num_zc = 0; > > > int orig_size; > > > @@ -971,6 +971,9 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > > > } > > > } > > > > > > + if (!msg_data_left(msg) && eor) > > > + goto just_flush; > > > + > > > > Hi David, > > > > the flow of this function is not entirely simple, so it is not easy for me > > to manually verify this. But in combination gcc-12 -Wmaybe-uninitialized > > and Smatch report that the following may be used uninitialised as a result > > of this change: > > > > * msg_pl > > This warning seems correct to me. > > > * orig_size > > This warning assumes we hit the first warning and then hit the goto > wait_for_memory; > > > * msg_en > > I don't get this warning on my system but it's the same thing. Hit the > first warning then the goto wait_for_memory. > > > * required_size > > Same. > > > * try_to_copy > > I don't really understand this warning and I can't reproduce it. > Strange. Thanks Dan. Of the above I think only the last one was flagged by GCC but not Smatch. I can try investigating further if it is useful.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index cac1adc968e8..6aa6d17888f5 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -945,7 +945,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) struct tls_rec *rec; int required_size; int num_async = 0; - bool full_record; + bool full_record = false; int record_room; int num_zc = 0; int orig_size; @@ -971,6 +971,9 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) } } + if (!msg_data_left(msg) && eor) + goto just_flush; + while (msg_data_left(msg)) { if (sk->sk_err) { ret = -sk->sk_err; @@ -1082,6 +1085,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) */ tls_ctx->pending_open_record_frags = true; copied += try_to_copy; +just_flush: if (full_record || eor) { ret = bpf_exec_tx_verdict(msg_pl, sk, full_record, record_type, &copied,
Allow userspace to end a TLS record without supplying any data by calling send()/sendto()/sendmsg() with no data and no MSG_MORE flag. This can be used to flush a previous send/splice that had MSG_MORE or SPLICE_F_MORE set or a sendfile() that was incomplete. Without this, a zero-length send to tls-sw is just ignored. I think tls-device will do the right thing without modification. Signed-off-by: David Howells <dhowells@redhat.com> cc: Chuck Lever <chuck.lever@oracle.com> cc: Boris Pismenny <borisp@nvidia.com> cc: John Fastabend <john.fastabend@gmail.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Eric Dumazet <edumazet@google.com> cc: "David S. Miller" <davem@davemloft.net> cc: Paolo Abeni <pabeni@redhat.com> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: netdev@vger.kernel.org --- net/tls/tls_sw.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)