Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
flat view
From: John Fastabend <john.fastabend@gmail.com>
Date: 2018-03-27 03:16:09
On 03/25/2018 11:54 PM, Prashant Bhole wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
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;
}
quoted hunk ↗ jump to hunk
+ } /* 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