| Message ID | 156023370286.5966.10762957456071886488.stgit@ubuntu-kvm1 |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: tls, correctly account for copied bytes with multiple sk_msgs | expand |
On 11.06.2019 08:15, John Fastabend wrote: > tls_sw_do_sendpage needs to return the total number of bytes sent > regardless of how many sk_msgs are allocatedt. Unfortunately, copied ^ typo > (the value we return up the stack) is zero'd before each new sk_msg > is alloced so we only return the copied size of the last sk_msg used. > > The application will then believe only part of its data was sent and > send the missing chunks again. However, because the data actually was > sent the receiver will get multiple copies of the same data. This description doesnt make sense to me as in my testing corruption occurs even when sendfile is always returning that it sent all the bytes requested. So all this resending(?) likely happens within the kernel. The fix does appear to work just fine however. Tested-by: Andre Tomt <andre@tomt.net> > To reproduce this do multiple copies close to the max record size to > force the above scenario. Andre created a C program that can easily > generate this case so we will push a similar selftest for this to > bpf-next shortly. > > The fix is to _not_ zero the copied field so that the total sent > bytes is returned. > > Reported-by: Steinar H. Gunderson <steinar+kernel@gunderson.no> > Reported-by: Andre Tomt <andre@tomt.net> > Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- > net/tls/tls_sw.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index d93f83f77864..5fe3dfa2c5e3 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1143,7 +1143,6 @@ static int tls_sw_do_sendpage(struct sock *sk, struct page *page, > > full_record = false; > record_room = TLS_MAX_PAYLOAD_SIZE - msg_pl->sg.size; > - copied = 0; > copy = size; > if (copy >= record_room) { > copy = record_room; >
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index d93f83f77864..5fe3dfa2c5e3 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1143,7 +1143,6 @@ static int tls_sw_do_sendpage(struct sock *sk, struct page *page, full_record = false; record_room = TLS_MAX_PAYLOAD_SIZE - msg_pl->sg.size; - copied = 0; copy = size; if (copy >= record_room) { copy = record_room;
tls_sw_do_sendpage needs to return the total number of bytes sent regardless of how many sk_msgs are allocatedt. Unfortunately, copied (the value we return up the stack) is zero'd before each new sk_msg is alloced so we only return the copied size of the last sk_msg used. The application will then believe only part of its data was sent and send the missing chunks again. However, because the data actually was sent the receiver will get multiple copies of the same data. To reproduce this do multiple copies close to the max record size to force the above scenario. Andre created a C program that can easily generate this case so we will push a similar selftest for this to bpf-next shortly. The fix is to _not_ zero the copied field so that the total sent bytes is returned. Reported-by: Steinar H. Gunderson <steinar+kernel@gunderson.no> Reported-by: Andre Tomt <andre@tomt.net> Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- net/tls/tls_sw.c | 1 - 1 file changed, 1 deletion(-)