bpf: sockmap, zero sg_size on error when buffer is released

When an error occurs during a redirect we have two cases that need
to be handled (i) we have a cork'ed buffer (ii) we have a normal
sendmsg buffer.

In the cork'ed buffer case we don't currently support recovering from
errors in a redirect action. So the buffer is released and the error
should _not_ be pushed back to the caller of sendmsg/sendpage. The
rationale here is the user will get an error that relates to old
data that may have been sent by some arbitrary thread on that sock.
Instead we simple consume the data and tell the user that the data
has been consumed. We may add proper error recovery in the future.
However, this patch fixes a bug where the bytes outstanding counter
sg_size was not zeroed. This could result in a case where if the user
has both a cork'ed action and apply action in progress we may
incorrectly call into the BPF program when the user expected an
old verdict to be applied via the apply action. I don't have a use
case where using apply and cork at the same time is valid but we
never explicitly reject it because it should work fine. This patch
ensures the sg_size is zeroed so we don't have this case.

In the normal sendmsg buffer case (no cork data) we also do not
zero sg_size. Again this can confuse the apply logic when the logic
calls into the BPF program when the BPF programmer expected the old
verdict to remain. So ensure we set sg_size to zero here as well. And
additionally to keep the psock state in-sync with the sk_msg_buff
release all the memory as well. Previously we did this before
returning to the user but this left a gap where psock and sk_msg_buff
states were out of sync which seems fragile. No additional overhead
is taken here except for a call to check the length and realize its
already been freed. This is in the error path as well so in my
opinion lets have robust code over optimized error paths.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 943929a05c923f638587f3e4b7f5f1b302b27b26..052c313b12dbf970c8d5bc5db53c11d5dca834d4 100644 (file)
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -701,15 +701,22 @@ more_data:
                err = bpf_tcp_sendmsg_do_redirect(redir, send, m, flags);
                lock_sock(sk);
 
+               if (unlikely(err < 0)) {
+                       free_start_sg(sk, m);
+                       psock->sg_size = 0;
+                       if (!cork)
+                               *copied -= send;
+               } else {
+                       psock->sg_size -= send;
+               }
+
                if (cork) {
                        free_start_sg(sk, m);
+                       psock->sg_size = 0;
                        kfree(m);
                        m = NULL;
+                       err = 0;
                }
-               if (unlikely(err))
-                       *copied -= err;
-               else
-                       psock->sg_size -= send;
                break;
        case __SK_DROP:
        default: