From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexei Starovoitov Subject: Re: [bpf PATCH 3/3] bpf: sockmap, fix error handling in redirect failures Date: Wed, 2 May 2018 12:34:41 -0700 Message-ID: <20180502193439.4r6hudjbaitlfyoi@ast-mbp> References: <20180502173954.17875.19624.stgit@john-Precision-Tower-5810> <20180502174737.17875.74031.stgit@john-Precision-Tower-5810> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: borkmann@iogearbox.net, ast@kernel.org, netdev@vger.kernel.org To: John Fastabend Return-path: Received: from mail-pg0-f67.google.com ([74.125.83.67]:45402 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750945AbeEBTeo (ORCPT ); Wed, 2 May 2018 15:34:44 -0400 Received: by mail-pg0-f67.google.com with SMTP id i29-v6so11341703pgn.12 for ; Wed, 02 May 2018 12:34:44 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180502174737.17875.74031.stgit@john-Precision-Tower-5810> Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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.