[PATCH v2 bpf-next 0/2] sockmap: fix sg api usage

[PATCH v2 bpf-next 0/2] sockmap: fix sg api usage

[Prashant Bhole]

These patches fix sg api usage in sockmap. Previously sockmap didn't
use sg_init_table(), which caused hitting BUG_ON in sg api, when
CONFIG_DEBUG_SG is enabled

v1: added sg_init_table() calls wherever needed.

v2:
- Patch1 adds new helper function in sg api. sg_init_marker()
- Patch2 sg_init_marker() and sg_init_table() in appropriate places

Backgroud:
While reviewing v1, John Fastabend raised a valid point about
unnecessary memset in sg_init_table() because sockmap uses sg table
which embedded in a struct. As enclosing struct is zeroed out, there
is unnecessary memset in sg_init_table.

So Daniel Borkmann suggested to define another static inline function
in scatterlist.h which only initializes sg_magic. Also this function 
will be called from sg_init_table. From this suggestion I defined a
function sg_init_marker() which sets sg_magic and calls sg_mark_end()

Prashant Bhole (2):
  lib/scatterlist: add sg_init_marker() helper
  bpf: sockmap: initialize sg table entries properly

 include/linux/scatterlist.h | 18 ++++++++++++++++++
 kernel/bpf/sockmap.c        | 13 ++++++++-----
 lib/scatterlist.c           |  9 +--------
 3 files changed, 27 insertions(+), 13 deletions(-)

-- 
2.14.3

[PATCH v2 bpf-next 1/2] lib/scatterlist: add sg_init_marker() helper

[Prashant Bhole]

sg_init_marker initializes sg_magic in the sg table and calls
sg_mark_end() on the last entry of the table. This can be useful to
avoid memset in sg_init_table() when scatterlist is already zeroed out

For example: when scatterlist is embedded inside other struct and that
container struct is zeroed out

Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 include/linux/scatterlist.h | 18 ++++++++++++++++++
 lib/scatterlist.c           |  9 +--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 22b2131bcdcd..aa5d4eb725f5 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -248,6 +248,24 @@ static inline void *sg_virt(struct scatterlist *sg)
 	return page_address(sg_page(sg)) + sg->offset;
 }
 
+/**
+ * sg_init_marker - Initialize markers in sg table
+ * @sgl:	   The SG table
+ * @nents:	   Number of entries in table
+ *
+ **/
+static inline void sg_init_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
+	sg_mark_end(&sgl[nents - 1]);
+}
+
 int sg_nents(struct scatterlist *sg);
 int sg_nents_for_len(struct scatterlist *sg, u64 len);
 struct scatterlist *sg_next(struct scatterlist *);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 53728d391d3a..06dad7a072fd 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -132,14 +132,7 @@ EXPORT_SYMBOL(sg_last);
 void sg_init_table(struct scatterlist *sgl, unsigned int nents)
 {
 	memset(sgl, 0, sizeof(*sgl) * nents);
-#ifdef CONFIG_DEBUG_SG
-	{
-		unsigned int i;
-		for (i = 0; i < nents; i++)
-			sgl[i].sg_magic = SG_MAGIC;
-	}
-#endif
-	sg_mark_end(&sgl[nents - 1]);
+	sg_init_marker(sgl, nents);
 }
 EXPORT_SYMBOL(sg_init_table);
 
-- 
2.14.3

Re: [PATCH v2 bpf-next 1/2] lib/scatterlist: add sg_init_marker() helper

[John Fastabend]

On 03/29/2018 05:20 PM, Prashant Bhole wrote:
> sg_init_marker initializes sg_magic in the sg table and calls
> sg_mark_end() on the last entry of the table. This can be useful to
> avoid memset in sg_init_table() when scatterlist is already zeroed out
> 
> For example: when scatterlist is embedded inside other struct and that
> container struct is zeroed out
> 
> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH v2 bpf-next 2/2] bpf: sockmap: initialize sg table entries properly

[Prashant Bhole]

When CONFIG_DEBUG_SG is set, sg->sg_magic is initialized in
sg_init_table() and it is verified in sg api while navigating. We hit
BUG_ON when magic check is failed.

In functions sg_tcp_sendpage and sg_tcp_sendmsg, the struct containing
the scatterlist is already zeroed out. So to avoid extra memset, we
use sg_init_marker() to initialize sg_magic.

Fixed following things:
- In bpf_tcp_sendpage: initialize sg using sg_init_marker
- In bpf_tcp_sendmsg: Replace sg_init_table with sg_init_marker
- In bpf_tcp_push: Replace memset with sg_init_table where consumed
  sg entry needs to be re-initialized.

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 kernel/bpf/sockmap.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 69c5bccabd22..b4f01656c452 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;
@@ -656,7 +656,7 @@ static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	}
 
 	sg = md.sg_data;
-	sg_init_table(sg, MAX_SKB_FRAGS);
+	sg_init_marker(sg, MAX_SKB_FRAGS);
 	rcu_read_unlock();
 
 	lock_sock(sk);
@@ -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_marker(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

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

[John Fastabend]

On 03/29/2018 05:21 PM, Prashant Bhole wrote:
> When CONFIG_DEBUG_SG is set, sg->sg_magic is initialized in
> sg_init_table() and it is verified in sg api while navigating. We hit
> BUG_ON when magic check is failed.
> 
> In functions sg_tcp_sendpage and sg_tcp_sendmsg, the struct containing
> the scatterlist is already zeroed out. So to avoid extra memset, we
> use sg_init_marker() to initialize sg_magic.
> 
> Fixed following things:
> - In bpf_tcp_sendpage: initialize sg using sg_init_marker
> - In bpf_tcp_sendmsg: Replace sg_init_table with sg_init_marker
> - In bpf_tcp_push: Replace memset with sg_init_table where consumed
>   sg entry needs to be re-initialized.
> 
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
>  kernel/bpf/sockmap.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH v2 bpf-next 0/2] sockmap: fix sg api usage

[John Fastabend]

On 03/29/2018 05:20 PM, Prashant Bhole wrote:
> These patches fix sg api usage in sockmap. Previously sockmap didn't
> use sg_init_table(), which caused hitting BUG_ON in sg api, when
> CONFIG_DEBUG_SG is enabled
> 
> v1: added sg_init_table() calls wherever needed.
> 
> v2:
> - Patch1 adds new helper function in sg api. sg_init_marker()
> - Patch2 sg_init_marker() and sg_init_table() in appropriate places
> 
> Backgroud:
> While reviewing v1, John Fastabend raised a valid point about
> unnecessary memset in sg_init_table() because sockmap uses sg table
> which embedded in a struct. As enclosing struct is zeroed out, there
> is unnecessary memset in sg_init_table.
> 
> So Daniel Borkmann suggested to define another static inline function
> in scatterlist.h which only initializes sg_magic. Also this function 
> will be called from sg_init_table. From this suggestion I defined a
> function sg_init_marker() which sets sg_magic and calls sg_mark_end()
> 

Series looks good to me thanks for finding and fixing this!

Re: [PATCH v2 bpf-next 0/2] sockmap: fix sg api usage

[Daniel Borkmann]

On 03/30/2018 05:39 AM, John Fastabend wrote:
> On 03/29/2018 05:20 PM, Prashant Bhole wrote:
>> These patches fix sg api usage in sockmap. Previously sockmap didn't
>> use sg_init_table(), which caused hitting BUG_ON in sg api, when
>> CONFIG_DEBUG_SG is enabled
>>
>> v1: added sg_init_table() calls wherever needed.
>>
>> v2:
>> - Patch1 adds new helper function in sg api. sg_init_marker()
>> - Patch2 sg_init_marker() and sg_init_table() in appropriate places
>>
>> Backgroud:
>> While reviewing v1, John Fastabend raised a valid point about
>> unnecessary memset in sg_init_table() because sockmap uses sg table
>> which embedded in a struct. As enclosing struct is zeroed out, there
>> is unnecessary memset in sg_init_table.
>>
>> So Daniel Borkmann suggested to define another static inline function
>> in scatterlist.h which only initializes sg_magic. Also this function 
>> will be called from sg_init_table. From this suggestion I defined a
>> function sg_init_marker() which sets sg_magic and calls sg_mark_end()
> 
> Series looks good to me thanks for finding and fixing this!

Applied to bpf-next, thanks Prashant!