netdev - Re: [bpf PATCH 3/3] bpf: sockmap, fix error handling in redirect failures

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180502193439.4r6hudjbaitlfyoi@ast-mbp>
Date:   Wed, 2 May 2018 12:34:41 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     borkmann@...earbox.net, ast@...nel.org, netdev@...r.kernel.org
Subject: Re: [bpf PATCH 3/3] bpf: sockmap, fix error handling in redirect
 failures

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.fastabend@...il.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.