Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly

[Daniel Borkmann <daniel@iogearbox.net>]

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 <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.
> 
> 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.

> -Prashant
>