| Message ID | 157851818932.1732.14521897338802839226.stgit@ubuntu3-kvm2 |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Fixes for sockmap/tls from more complex BPF progs | expand |
On 8 Jan 2020, at 13:16, John Fastabend wrote: > When user returns SK_DROP we need to reset the number of copied bytes > to indicate to the user the bytes were dropped and not sent. If we > don't reset the copied arg sendmsg will return as if those bytes were > copied giving the user a positive return value. > > This works as expected today except in the case where the user also > pops bytes. In the pop case the sg.size is reduced but we don't correctly > account for this when copied bytes is reset. The popped bytes are not > accounted for and we return a small positive value potentially confusing > the user. > > The reason this happens is due to a typo where we do the wrong comparison > when accounting for pop bytes. In this fix notice the if/else is not > needed and that we have a similar problem if we push data except its not > visible to the user because if delta is larger the sg.size we return a > negative value so it appears as an error regardless. > > Fixes: 7246d8ed4dcce ("bpf: helper to pop data from messages") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index e38705165ac9..587d55611814 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -315,10 +315,7 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock, */ delta = msg->sg.size; psock->eval = sk_psock_msg_verdict(sk, psock, msg); - if (msg->sg.size < delta) - delta -= msg->sg.size; - else - delta = 0; + delta -= msg->sg.size; } if (msg->cork_bytes && diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 0326e916ab01..d9a757c0ba0c 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -812,10 +812,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk, if (psock->eval == __SK_NONE) { delta = msg->sg.size; psock->eval = sk_psock_msg_verdict(sk, psock, msg); - if (delta < msg->sg.size) - delta -= msg->sg.size; - else - delta = 0; + delta -= msg->sg.size; } if (msg->cork_bytes && msg->cork_bytes > msg->sg.size && !enospc && !full_record) {
When user returns SK_DROP we need to reset the number of copied bytes to indicate to the user the bytes were dropped and not sent. If we don't reset the copied arg sendmsg will return as if those bytes were copied giving the user a positive return value. This works as expected today except in the case where the user also pops bytes. In the pop case the sg.size is reduced but we don't correctly account for this when copied bytes is reset. The popped bytes are not accounted for and we return a small positive value potentially confusing the user. The reason this happens is due to a typo where we do the wrong comparison when accounting for pop bytes. In this fix notice the if/else is not needed and that we have a similar problem if we push data except its not visible to the user because if delta is larger the sg.size we return a negative value so it appears as an error regardless. Fixes: 7246d8ed4dcce ("bpf: helper to pop data from messages") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- net/ipv4/tcp_bpf.c | 5 +---- net/tls/tls_sw.c | 5 +---- 2 files changed, 2 insertions(+), 8 deletions(-)