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