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

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);
 
 			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

On 03/25/2018 11:54 PM, Prashant Bhole wrote:

Looks OK here.

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;
}

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

On 3/27/2018 12:15 PM, John Fastabend wrote:

On 03/25/2018 11:54 PM, Prashant Bhole wrote:

quoted

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

  
  			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

On 03/27/2018 10:41 AM, Prashant Bhole wrote:

On 3/27/2018 12:15 PM, John Fastabend wrote:

quoted

On 03/25/2018 11:54 PM, Prashant Bhole wrote:

quoted

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

                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

On 3/27/2018 6:05 PM, Daniel Borkmann wrote:

On 03/27/2018 10:41 AM, Prashant Bhole wrote:

quoted

On 3/27/2018 12:15 PM, John Fastabend wrote:

quoted

On 03/25/2018 11:54 PM, Prashant Bhole wrote:

quoted

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

                 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

On 03/28/2018 08:18 AM, Prashant Bhole wrote:

On 3/27/2018 6:05 PM, Daniel Borkmann wrote:

quoted

On 03/27/2018 10:41 AM, Prashant Bhole wrote:

quoted

On 3/27/2018 12:15 PM, John Fastabend wrote:

quoted

On 03/25/2018 11:54 PM, Prashant Bhole wrote:

quoted

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

                 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

On 3/28/2018 5:51 PM, Daniel Borkmann wrote:

On 03/28/2018 08:18 AM, Prashant Bhole wrote:

quoted

On 3/27/2018 6:05 PM, Daniel Borkmann wrote:

quoted

On 03/27/2018 10:41 AM, Prashant Bhole wrote:

quoted

On 3/27/2018 12:15 PM, John Fastabend wrote:

quoted

On 03/25/2018 11:54 PM, Prashant Bhole wrote:

quoted

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

                  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.

quoted

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