On Wed, May 02, 2018 at 10:47:37AM -0700, John Fastabend wrote:
> When a redirect failure happens we release the buffers in-flight
> without calling a sk_mem_uncharge(), the uncharge is called before
> dropping the sock lock for the redirecte, however we missed updating
> the ring start index. When no apply actions are in progress this
> is OK because we uncharge the entire buffer before the redirect.
> But, when we have apply logic running its possible that only a
> portion of the buffer is being redirected. In this case we only
> do memory accounting for the buffer slice being redirected and
> expect to be able to loop over the BPF program again and/or if
> a sock is closed uncharge the memory at sock destruct time.
>
> With an invalid start index however the program logic looks at
> the start pointer index, checks the length, and when seeing the
> length is zero (from the initial release and failure to update
> the pointer) aborts without uncharging/releasing the remaining
> memory.
>
> The fix for this is simply to update the start index. To avoid
> fixing this error in two locations we do a small refactor and
> remove one case where it is open-coded. Then fix it in the
> single function.
>
> Signed-off-by: John Fastabend <john.fastab...@gmail.com>
> ---
> kernel/bpf/sockmap.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index 052c313..7e3c4cd 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -393,7 +393,8 @@ static void return_mem_sg(struct sock *sk, int bytes,
> struct sk_msg_buff *md)
> } while (i != md->sg_end);
> }
>
> -static void free_bytes_sg(struct sock *sk, int bytes, struct sk_msg_buff *md)
> +static void free_bytes_sg(struct sock *sk, int bytes,
> + struct sk_msg_buff *md, bool charge)
> {
> struct scatterlist *sg = md->sg_data;
> int i = md->sg_start, free;
> @@ -403,11 +404,13 @@ static void free_bytes_sg(struct sock *sk, int bytes,
> struct sk_msg_buff *md)
> if (bytes < free) {
> sg[i].length -= bytes;
> sg[i].offset += bytes;
> - sk_mem_uncharge(sk, bytes);
> + if (charge)
> + sk_mem_uncharge(sk, bytes);
> break;
> }
>
> - sk_mem_uncharge(sk, sg[i].length);
> + if (charge)
> + sk_mem_uncharge(sk, sg[i].length);
> put_page(sg_page(&sg[i]));
> bytes -= sg[i].length;
> sg[i].length = 0;
> @@ -418,6 +421,7 @@ static void free_bytes_sg(struct sock *sk, int bytes,
> struct sk_msg_buff *md)
> if (i == MAX_SKB_FRAGS)
> i = 0;
> }
> + md->sg_start = i;
> }
>
> static int free_sg(struct sock *sk, int start, struct sk_msg_buff *md)
> @@ -578,7 +582,7 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk,
> int send,
> {
> struct smap_psock *psock;
> struct scatterlist *sg;
> - int i, err, free = 0;
> + int i, err = 0;
> bool ingress = !!(md->flags & BPF_F_INGRESS);
>
> sg = md->sg_data;
> @@ -607,16 +611,8 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk,
> int send,
> out_rcu:
> rcu_read_unlock();
> out:
> - i = md->sg_start;
> - while (sg[i].length) {
> - free += sg[i].length;
> - put_page(sg_page(&sg[i]));
> - sg[i].length = 0;
> - i++;
> - if (i == MAX_SKB_FRAGS)
> - i = 0;
> - }
this hunk is causing:
../kernel/bpf/sockmap.c:585:6: warning: unused variable ‘i’ [-Wunused-variable]
int i, err = 0;
please respin.
