From mboxrd@z Thu Jan 1 00:00:00 1970 From: Prashant Bhole Subject: Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly Date: Fri, 30 Mar 2018 09:20:10 +0900 Message-ID: References: <20180326065443.7880-1-bhole_prashant_q7@lab.ntt.co.jp> <87c1e6e0-913f-2297-986f-f6b70ce1e485@lab.ntt.co.jp> <92d66b9e-d93a-9f87-a6db-84aee0c14284@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: John Fastabend , Alexei Starovoitov , "David S . Miller" , netdev@vger.kernel.org To: Daniel Borkmann Return-path: Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:53909 "EHLO tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751182AbeC3AUp (ORCPT ); Thu, 29 Mar 2018 20:20:45 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 3/28/2018 5:51 PM, Daniel Borkmann wrote: > On 03/28/2018 08:18 AM, Prashant Bhole wrote: >> On 3/27/2018 6:05 PM, Daniel Borkmann wrote: >>> On 03/27/2018 10:41 AM, Prashant Bhole wrote: >>>> On 3/27/2018 12:15 PM, John Fastabend wrote: >>>>> 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; >>>>> } >>>> >>>> Similar sg_init_table() is present in bpf_tcp_sendmsg(). >>>> I agree that it causes unnecessary memset, but I don't agree with open coded fix. >>> >>> But then lets fix is properly and add a static inline helper to the >>> include/linux/scatterlist.h header like ... >>> >>> static inline void sg_init_debug_marker(struct scatterlist *sgl, >>>                     unsigned int nents) >>> { >>> #ifdef CONFIG_DEBUG_SG >>>     unsigned int i; >>> >>>     for (i = 0; i < nents; i++) >>>         sgl[i].sg_magic = SG_MAGIC; >>> #endif >>> } >>> >>> ... and reuse it in all the places that would otherwise open-code this, >>> as well as sg_init_table(): >>> >>> void sg_init_table(struct scatterlist *sgl, unsigned int nents) >>> { >>>          memset(sgl, 0, sizeof(*sgl) * nents); >>>     sg_init_debug_marker(sgl, nents); >>>          sg_mark_end(&sgl[nents - 1]); >>> } >>> >>> This would be a lot cleaner than having this duplicated in various places. >> >> Daniel, This is a good suggestion. Is it ok if I submit both changes in >> a patch series? > > Sure, that's fine. > >> How scatterlist related changes will be picked up by other subsystems? > > Once this gets applied into bpf-next, this will be pushed to net-next tree, > and during the merge window net-next will be pulled into Linus' tree if this > is what you are asking. Then also other subsystems outside of bpf/networking > can make use of the sg_init_debug_marker() helper if suitable for their > situation. Thanks. I am submitting V2 soon. -Prashant