From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly Date: Mon, 26 Mar 2018 20:15:55 -0700 Message-ID: References: <20180326065443.7880-1-bhole_prashant_q7@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org To: Prashant Bhole , Daniel Borkmann , Alexei Starovoitov , "David S . Miller" Return-path: Received: from mail-pg0-f67.google.com ([74.125.83.67]:37275 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751132AbeC0DQJ (ORCPT ); Mon, 26 Mar 2018 23:16:09 -0400 Received: by mail-pg0-f67.google.com with SMTP id n11so8066373pgp.4 for ; Mon, 26 Mar 2018 20:16:09 -0700 (PDT) In-Reply-To: <20180326065443.7880-1-bhole_prashant_q7@lab.ntt.co.jp> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 03/25/2018 11:54 PM, Prashant Bhole wrote: > When CONFIG_DEBUG_SG is set, sg->sg_magic is initialized to SG_MAGIC, > when sg table is initialized using sg_init_table(). Magic is checked > while navigating the scatterlist. We hit BUG_ON when magic check is > failed. > > Fixed following things: > - Initialization of sg table in bpf_tcp_sendpage() was missing, > initialized it using sg_init_table() > > - bpf_tcp_sendmsg() initializes sg table using sg_init_table() before > entering the loop, but further consumed sg entries are initialized > using memset. Fixed it by replacing memset with sg_init_table() in > function bpf_tcp_push() > > Signed-off-by: Prashant Bhole > --- > kernel/bpf/sockmap.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c > index 69c5bccabd22..8a848a99d768 100644 > --- a/kernel/bpf/sockmap.c > +++ b/kernel/bpf/sockmap.c > @@ -312,7 +312,7 @@ static int bpf_tcp_push(struct sock *sk, int apply_bytes, > md->sg_start++; > if (md->sg_start == MAX_SKB_FRAGS) > md->sg_start = 0; > - memset(sg, 0, sizeof(*sg)); > + sg_init_table(sg, 1); Looks OK here. > > if (md->sg_start == md->sg_end) > break; > @@ -763,10 +763,14 @@ static int bpf_tcp_sendpage(struct sock *sk, struct page *page, > > lock_sock(sk); > > - if (psock->cork_bytes) > + if (psock->cork_bytes) { > m = psock->cork; > - else > + sg = &m->sg_data[m->sg_end]; > + } else { > m = &md; > + sg = m->sg_data; > + sg_init_table(sg, MAX_SKB_FRAGS); sg_init_table() does an unnecessary memset() though. We probably either want a new scatterlist API or just open code this, #ifdef CONFIG_DEBUG_SG { unsigned int i; for (i = 0; i < nents; i++) sgl[i].sg_magic = SG_MAGIC; } > + } > > /* Catch case where ring is full and sendpage is stalled. */ > if (unlikely(m->sg_end == m->sg_start && > @@ -774,7 +778,6 @@ static int bpf_tcp_sendpage(struct sock *sk, struct page *page, > goto out_err; > > psock->sg_size += size; > - sg = &m->sg_data[m->sg_end]; > sg_set_page(sg, page, size, offset); > get_page(page); > m->sg_copy[m->sg_end] = true; > Nice, catch. I probably should audit though code paths as well and run the test suite with CONFIG_DEBUG_SG. There might be a couple other spots where I open coded the sg elements. Thanks, John