From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries
 properly
Date: Tue, 27 Mar 2018 11:05:11 +0200
Message-ID: <c120c2f6-237c-0fbf-4069-2770cedf38e6@iogearbox.net>
References: <20180326065443.7880-1-bhole_prashant_q7@lab.ntt.co.jp>
 <dd0531c0-9216-89db-6752-213401392705@gmail.com>
 <87c1e6e0-913f-2297-986f-f6b70ce1e485@lab.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: Alexei Starovoitov <ast@kernel.org>,
        "David S . Miller" <davem@davemloft.net>, netdev@vger.kernel.org
To: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>,
        John Fastabend <john.fastabend@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from www62.your-server.de ([213.133.104.62]:43224 "EHLO
        www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751001AbeC0JFR (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 27 Mar 2018 05:05:17 -0400
In-Reply-To: <87c1e6e0-913f-2297-986f-f6b70ce1e485@lab.ntt.co.jp>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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 <bhole_prashant_q7@lab.ntt.co.jp>
>>> ---
>>>   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.

Thanks,
Daniel