Thread (7 messages) 7 messages, 3 authors, 2018-03-30

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help