Commit 501ca817 authored by John Fastabend's avatar John Fastabend Committed by Daniel Borkmann

bpf: sockmap, decrement copied count correctly in redirect error case

Currently, when a redirect occurs in sockmap and an error occurs in
the redirect call we unwind the scatterlist once in the error path
of bpf_tcp_sendmsg_do_redirect() and then again in sendmsg(). Then
in the error path of sendmsg we decrement the copied count by the
send size.

However, its possible we partially sent data before the error was
generated. This can happen if do_tcp_sendpages() partially sends the
scatterlist before encountering a memory pressure error. If this
happens we need to decrement the copied value (the value tracking
how many bytes were actually sent to TCP stack) by the number of
remaining bytes _not_ the entire send size. Otherwise we risk
confusing userspace.

Also we don't need two calls to free the scatterlist one is
good enough. So remove the one in bpf_tcp_sendmsg_do_redirect() and
then properly reduce copied by the number of remaining bytes which
may in fact be the entire send size if no bytes were sent.

To do this use bool to indicate if free_start_sg() should do mem
accounting or not.
Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
parent 3f6e138d
...@@ -236,7 +236,7 @@ static int bpf_tcp_init(struct sock *sk) ...@@ -236,7 +236,7 @@ static int bpf_tcp_init(struct sock *sk)
} }
static void smap_release_sock(struct smap_psock *psock, struct sock *sock); static void smap_release_sock(struct smap_psock *psock, struct sock *sock);
static int free_start_sg(struct sock *sk, struct sk_msg_buff *md); static int free_start_sg(struct sock *sk, struct sk_msg_buff *md, bool charge);
static void bpf_tcp_release(struct sock *sk) static void bpf_tcp_release(struct sock *sk)
{ {
...@@ -248,7 +248,7 @@ static void bpf_tcp_release(struct sock *sk) ...@@ -248,7 +248,7 @@ static void bpf_tcp_release(struct sock *sk)
goto out; goto out;
if (psock->cork) { if (psock->cork) {
free_start_sg(psock->sock, psock->cork); free_start_sg(psock->sock, psock->cork, true);
kfree(psock->cork); kfree(psock->cork);
psock->cork = NULL; psock->cork = NULL;
} }
...@@ -330,14 +330,14 @@ static void bpf_tcp_close(struct sock *sk, long timeout) ...@@ -330,14 +330,14 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
close_fun = psock->save_close; close_fun = psock->save_close;
if (psock->cork) { if (psock->cork) {
free_start_sg(psock->sock, psock->cork); free_start_sg(psock->sock, psock->cork, true);
kfree(psock->cork); kfree(psock->cork);
psock->cork = NULL; psock->cork = NULL;
} }
list_for_each_entry_safe(md, mtmp, &psock->ingress, list) { list_for_each_entry_safe(md, mtmp, &psock->ingress, list) {
list_del(&md->list); list_del(&md->list);
free_start_sg(psock->sock, md); free_start_sg(psock->sock, md, true);
kfree(md); kfree(md);
} }
...@@ -570,14 +570,16 @@ static void free_bytes_sg(struct sock *sk, int bytes, ...@@ -570,14 +570,16 @@ static void free_bytes_sg(struct sock *sk, int bytes,
md->sg_start = i; md->sg_start = i;
} }
static int free_sg(struct sock *sk, int start, struct sk_msg_buff *md) static int free_sg(struct sock *sk, int start,
struct sk_msg_buff *md, bool charge)
{ {
struct scatterlist *sg = md->sg_data; struct scatterlist *sg = md->sg_data;
int i = start, free = 0; int i = start, free = 0;
while (sg[i].length) { while (sg[i].length) {
free += sg[i].length; free += sg[i].length;
sk_mem_uncharge(sk, sg[i].length); if (charge)
sk_mem_uncharge(sk, sg[i].length);
if (!md->skb) if (!md->skb)
put_page(sg_page(&sg[i])); put_page(sg_page(&sg[i]));
sg[i].length = 0; sg[i].length = 0;
...@@ -594,9 +596,9 @@ static int free_sg(struct sock *sk, int start, struct sk_msg_buff *md) ...@@ -594,9 +596,9 @@ static int free_sg(struct sock *sk, int start, struct sk_msg_buff *md)
return free; return free;
} }
static int free_start_sg(struct sock *sk, struct sk_msg_buff *md) static int free_start_sg(struct sock *sk, struct sk_msg_buff *md, bool charge)
{ {
int free = free_sg(sk, md->sg_start, md); int free = free_sg(sk, md->sg_start, md, charge);
md->sg_start = md->sg_end; md->sg_start = md->sg_end;
return free; return free;
...@@ -604,7 +606,7 @@ static int free_start_sg(struct sock *sk, struct sk_msg_buff *md) ...@@ -604,7 +606,7 @@ static int free_start_sg(struct sock *sk, struct sk_msg_buff *md)
static int free_curr_sg(struct sock *sk, struct sk_msg_buff *md) static int free_curr_sg(struct sock *sk, struct sk_msg_buff *md)
{ {
return free_sg(sk, md->sg_curr, md); return free_sg(sk, md->sg_curr, md, true);
} }
static int bpf_map_msg_verdict(int _rc, struct sk_msg_buff *md) static int bpf_map_msg_verdict(int _rc, struct sk_msg_buff *md)
...@@ -718,7 +720,7 @@ static int bpf_tcp_ingress(struct sock *sk, int apply_bytes, ...@@ -718,7 +720,7 @@ static int bpf_tcp_ingress(struct sock *sk, int apply_bytes,
list_add_tail(&r->list, &psock->ingress); list_add_tail(&r->list, &psock->ingress);
sk->sk_data_ready(sk); sk->sk_data_ready(sk);
} else { } else {
free_start_sg(sk, r); free_start_sg(sk, r, true);
kfree(r); kfree(r);
} }
...@@ -752,14 +754,10 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send, ...@@ -752,14 +754,10 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
release_sock(sk); release_sock(sk);
} }
smap_release_sock(psock, sk); smap_release_sock(psock, sk);
if (unlikely(err)) return err;
goto out;
return 0;
out_rcu: out_rcu:
rcu_read_unlock(); rcu_read_unlock();
out: return 0;
free_bytes_sg(NULL, send, md, false);
return err;
} }
static inline void bpf_md_init(struct smap_psock *psock) static inline void bpf_md_init(struct smap_psock *psock)
...@@ -822,7 +820,7 @@ static int bpf_exec_tx_verdict(struct smap_psock *psock, ...@@ -822,7 +820,7 @@ static int bpf_exec_tx_verdict(struct smap_psock *psock,
case __SK_PASS: case __SK_PASS:
err = bpf_tcp_push(sk, send, m, flags, true); err = bpf_tcp_push(sk, send, m, flags, true);
if (unlikely(err)) { if (unlikely(err)) {
*copied -= free_start_sg(sk, m); *copied -= free_start_sg(sk, m, true);
break; break;
} }
...@@ -845,16 +843,17 @@ static int bpf_exec_tx_verdict(struct smap_psock *psock, ...@@ -845,16 +843,17 @@ static int bpf_exec_tx_verdict(struct smap_psock *psock,
lock_sock(sk); lock_sock(sk);
if (unlikely(err < 0)) { if (unlikely(err < 0)) {
free_start_sg(sk, m); int free = free_start_sg(sk, m, false);
psock->sg_size = 0; psock->sg_size = 0;
if (!cork) if (!cork)
*copied -= send; *copied -= free;
} else { } else {
psock->sg_size -= send; psock->sg_size -= send;
} }
if (cork) { if (cork) {
free_start_sg(sk, m); free_start_sg(sk, m, true);
psock->sg_size = 0; psock->sg_size = 0;
kfree(m); kfree(m);
m = NULL; m = NULL;
...@@ -1121,7 +1120,7 @@ static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) ...@@ -1121,7 +1120,7 @@ static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
err = sk_stream_wait_memory(sk, &timeo); err = sk_stream_wait_memory(sk, &timeo);
if (err) { if (err) {
if (m && m != psock->cork) if (m && m != psock->cork)
free_start_sg(sk, m); free_start_sg(sk, m, true);
goto out_err; goto out_err;
} }
} }
...@@ -1580,13 +1579,13 @@ static void smap_gc_work(struct work_struct *w) ...@@ -1580,13 +1579,13 @@ static void smap_gc_work(struct work_struct *w)
bpf_prog_put(psock->bpf_tx_msg); bpf_prog_put(psock->bpf_tx_msg);
if (psock->cork) { if (psock->cork) {
free_start_sg(psock->sock, psock->cork); free_start_sg(psock->sock, psock->cork, true);
kfree(psock->cork); kfree(psock->cork);
} }
list_for_each_entry_safe(md, mtmp, &psock->ingress, list) { list_for_each_entry_safe(md, mtmp, &psock->ingress, list) {
list_del(&md->list); list_del(&md->list);
free_start_sg(psock->sock, md); free_start_sg(psock->sock, md, true);
kfree(md); kfree(md);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment