* [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
@ 2018-03-26 6:54 Prashant Bhole
2018-03-27 3:15 ` John Fastabend
0 siblings, 1 reply; 7+ messages in thread
From: Prashant Bhole @ 2018-03-26 6:54 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, David S . Miller
Cc: Prashant Bhole, John Fastabend, netdev
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);
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);
+ }
/* 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;
--
2.14.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
2018-03-26 6:54 [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly Prashant Bhole
@ 2018-03-27 3:15 ` John Fastabend
2018-03-27 8:41 ` Prashant Bhole
0 siblings, 1 reply; 7+ messages in thread
From: John Fastabend @ 2018-03-27 3:15 UTC (permalink / raw)
To: Prashant Bhole, Daniel Borkmann, Alexei Starovoitov,
David S . Miller
Cc: netdev
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;
}
> + }
>
> /* 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
2018-03-27 3:15 ` John Fastabend
@ 2018-03-27 8:41 ` Prashant Bhole
2018-03-27 9:05 ` Daniel Borkmann
0 siblings, 1 reply; 7+ messages in thread
From: Prashant Bhole @ 2018-03-27 8:41 UTC (permalink / raw)
To: John Fastabend
Cc: Daniel Borkmann, Alexei Starovoitov, David S . Miller, netdev
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.
I am still with V1. Thanks.
-Prashant
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
2018-03-27 8:41 ` Prashant Bhole
@ 2018-03-27 9:05 ` Daniel Borkmann
2018-03-28 6:18 ` Prashant Bhole
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Borkmann @ 2018-03-27 9:05 UTC (permalink / raw)
To: Prashant Bhole, John Fastabend
Cc: Alexei Starovoitov, David S . Miller, netdev
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
2018-03-27 9:05 ` Daniel Borkmann
@ 2018-03-28 6:18 ` Prashant Bhole
2018-03-28 8:51 ` Daniel Borkmann
0 siblings, 1 reply; 7+ messages in thread
From: Prashant Bhole @ 2018-03-28 6:18 UTC (permalink / raw)
To: Daniel Borkmann
Cc: John Fastabend, Alexei Starovoitov, David S . Miller, netdev
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? How scatterlist related changes will be picked up by
other subsystems?
-Prashant
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
2018-03-28 6:18 ` Prashant Bhole
@ 2018-03-28 8:51 ` Daniel Borkmann
2018-03-30 0:20 ` Prashant Bhole
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Borkmann @ 2018-03-28 8:51 UTC (permalink / raw)
To: Prashant Bhole
Cc: John Fastabend, Alexei Starovoitov, David S . Miller, netdev
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
2018-03-28 8:51 ` Daniel Borkmann
@ 2018-03-30 0:20 ` Prashant Bhole
0 siblings, 0 replies; 7+ messages in thread
From: Prashant Bhole @ 2018-03-30 0:20 UTC (permalink / raw)
To: Daniel Borkmann
Cc: John Fastabend, Alexei Starovoitov, David S . Miller, netdev
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 <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.
Thanks. I am submitting V2 soon.
-Prashant
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-30 0:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-26 6:54 [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly Prashant Bhole
2018-03-27 3:15 ` John Fastabend
2018-03-27 8:41 ` Prashant Bhole
2018-03-27 9:05 ` Daniel Borkmann
2018-03-28 6:18 ` Prashant Bhole
2018-03-28 8:51 ` Daniel Borkmann
2018-03-30 0:20 ` Prashant Bhole
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox