oopses since "net: Optimize memory usage when splicing from sockets"

oopses since "net: Optimize memory usage when splicing from sockets"

[Lennert Buytenhek]

Since 4fb669948116d928ae44262ab7743732c574630d ("net: Optimize memory
usage when splicing from sockets.") I'm seeing this oops (e.g. in
2.6.30-rc3) when splicing from a TCP socket to /dev/null on a driver
(mv643xx_eth) that uses LRO in the skb mode (lro_receive_skb) rather
than the frag mode:

(sorry, truncated at 80 columns due to terminal emulator)


Unable to handle kernel NULL pointer dereference at virtual address 00000100
pgd = dfb54000
[00000100] *pgd=1fb86031, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1]
Modules linked in:
CPU: 0    Not tainted  (2.6.30-rc3-00485-g2ef470b-dirty #1451)
PC is at __skb_splice_bits+0xcc/0x3ac
LR is at skb_splice_bits+0xa0/0x118
pc : [<c023e440>]    lr : [<c023eb20>]    psr: 80000013
sp : de8e1d38  ip : c07fb880  fp : c07fb880
r10: dfb87a00  r9 : 000005a8  r8 : de8e1d70
r7 : 00000000  r6 : 000005a8  r5 : 00000864  r4 : de8e1e78
r3 : 0000079c  r2 : c0404000  r1 : 00000000  r0 : dfb896e0
Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0005397f  Table: 1fb54000  DAC: 00000015
Process discard_splice (pid: 1420, stack limit = 0xde8e0268)
Stack: (0xde8e1d38 to 0xde8e2000)
1d20:                                                       00000000 00000000
1d40: de8e1d74 dfb896e0 de8e1ea4 dfb896e0 dfb89640 de8e1e78 de8e1d74 de8e1d70
1d60: c0269b18 dfb87a00 000002f8 c023eb20 000005a8 00000000 00000000 000005a8
1d80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1da0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1dc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1de0: 00000000 00000000 00000000 00000000 00000000 de8e1e34 00000001 c01b9a4c
1e00: 0000013c 00000000 df41ec24 c0060724 df41ec20 dfbcfe38 dfbcfe38 00000000
1e20: 400c7000 c006f928 dfbcfe38 c00265c0 c03bf774 df9012ac c07f63a0 00000000
1e40: 00000000 df41ec34 0000008a 0000008a dfbcfe38 00000200 00000000 00000000
1e60: c07fe420 00000000 00000000 c0071b54 00000000 df9adb1c de8e1e38 de8e1d78
1e80: 00000001 00000002 c03baf6c c023e720 00000000 de8e1ef0 00100000 8b55851e
1ea0: 00000b50 de8e1ef0 df9284b8 c0269b44 00000002 dfb89640 00000000 df928460
1ec0: 00000000 c02685d8 00000000 df928460 00000000 de8e0000 de8e1ef0 00100000
1ee0: de8e1f00 df9284ec 00000000 c02687e4 00000000 00100000 de8e1f00 00000000
1f00: dfb87a00 00100000 00000002 00000000 c00ac128 df920a20 df920a40 00100000
1f20: dfb87a00 00100000 dfb87a00 0001537c 00000002 c02382c4 00000002 df920a40
1f40: dfb87a00 c009dcc0 00000002 df920a40 dfb87a00 00000000 df920a40 df920a20
1f60: dfbbaa20 c009ded4 00000002 00018068 de8e1f90 0000004e 00000000 00000000
1f80: 00000008 00100000 00000002 00000000 00000154 c0020a24 de8e0000 0001537c
1fa0: 00018018 c00208a0 00100000 00000002 00000008 00000000 00000005 00000000
1fc0: 00100000 00000002 00000000 00000154 00015384 00015380 0001537c 00018018
1fe0: 0001532c becf1af8 00008e5c 4011a928 60000010 00000008 00000000 00000000
[<c023e440>] (__skb_splice_bits+0xcc/0x3ac) from [<c023eb20>] (skb_splice_bits+)
[<c023eb20>] (skb_splice_bits+0xa0/0x118) from [<c0269b44>] (tcp_splice_data_re)
[<c0269b44>] (tcp_splice_data_recv+0x2c/0x40) from [<c02685d8>] (tcp_read_sock+)
[<c02685d8>] (tcp_read_sock+0x6c/0x1e4) from [<c02687e4>] (tcp_splice_read+0x94)
[<c02687e4>] (tcp_splice_read+0x94/0x1d8) from [<c02382c4>] (sock_splice_read+0)
[<c02382c4>] (sock_splice_read+0x28/0x2c) from [<c009dcc0>] (do_splice_to+0x78/)
[<c009dcc0>] (do_splice_to+0x78/0x84) from [<c009ded4>] (sys_splice+0x208/0x2a8)
[<c009ded4>] (sys_splice+0x208/0x2a8) from [<c00208a0>] (ret_fast_syscall+0x0/0)
Code: e2653a01 e5907008 e1560003 21a06003 (e597a100)
---[ end trace 82cf6824c91dfc15 ]---


addr2line suggests skb->sk is NULL in linear_to_page():


	static inline struct page *linear_to_page(struct page *page, unsigned int *len,
						  unsigned int *offset,
						  struct sk_buff *skb)
	{
		struct sock *sk = skb->sk;
		struct page *p = sk->sk_sndmsg_page;		<========
		unsigned int off;

		if (!p) {


When we get here, skb->sk has apparently already been dropped, leading
to a NULL pointer deref.  Backing out the offending commit makes the
oops go away (as does converting the driver to lro frag rx, but that
destroys routing performance).

Thoughts?  Should we just fall back to plain alloc_pages() if skb->sk
is NULL, or should have still have the socket reference when we get here?

Re: oopses since "net: Optimize memory usage when splicing from sockets"

[Jarek Poplawski]

On Thu, Apr 30, 2009 at 05:16:26AM +0200, Lennert Buytenhek wrote:
> Since 4fb669948116d928ae44262ab7743732c574630d ("net: Optimize memory
> usage when splicing from sockets.") I'm seeing this oops (e.g. in
> 2.6.30-rc3) when splicing from a TCP socket to /dev/null on a driver
> (mv643xx_eth) that uses LRO in the skb mode (lro_receive_skb) rather
> than the frag mode:
...
> addr2line suggests skb->sk is NULL in linear_to_page():
> 
> 
> 	static inline struct page *linear_to_page(struct page *page, unsigned int *len,
> 						  unsigned int *offset,
> 						  struct sk_buff *skb)
> 	{
> 		struct sock *sk = skb->sk;
> 		struct page *p = sk->sk_sndmsg_page;		<========
> 		unsigned int off;
> 
> 		if (!p) {
> 
> 
> When we get here, skb->sk has apparently already been dropped, leading
> to a NULL pointer deref.  Backing out the offending commit makes the
> oops go away (as does converting the driver to lro frag rx, but that
> destroys routing performance).
> 
> Thoughts?  Should we just fall back to plain alloc_pages() if skb->sk
> is NULL, or should have still have the socket reference when we get here?

Hmm... I definitely need more time for this, but the first and maybe
wrong impression is this is an skb from the frag_list. There are
probably better ways of fixing it properly, but here is a quick hack
for the beginning (alas not even compile-tested at the moment).

Thanks for debugging this,
Jarek P.
---

 net/core/skbuff.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ce6356c..f091a5a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1365,9 +1365,8 @@ static void sock_spd_release(struct splice_pipe_desc *spd, unsigned int i)
 
 static inline struct page *linear_to_page(struct page *page, unsigned int *len,
 					  unsigned int *offset,
-					  struct sk_buff *skb)
+					  struct sk_buff *skb, struct sock *sk)
 {
-	struct sock *sk = skb->sk;
 	struct page *p = sk->sk_sndmsg_page;
 	unsigned int off;
 
@@ -1405,13 +1404,14 @@ new_page:
  */
 static inline int spd_fill_page(struct splice_pipe_desc *spd, struct page *page,
 				unsigned int *len, unsigned int offset,
-				struct sk_buff *skb, int linear)
+				struct sk_buff *skb, int linear,
+				struct sock *sk)
 {
 	if (unlikely(spd->nr_pages == PIPE_BUFFERS))
 		return 1;
 
 	if (linear) {
-		page = linear_to_page(page, len, &offset, skb);
+		page = linear_to_page(page, len, &offset, skb, sk);
 		if (!page)
 			return 1;
 	} else
@@ -1442,7 +1442,8 @@ static inline void __segment_seek(struct page **page, unsigned int *poff,
 static inline int __splice_segment(struct page *page, unsigned int poff,
 				   unsigned int plen, unsigned int *off,
 				   unsigned int *len, struct sk_buff *skb,
-				   struct splice_pipe_desc *spd, int linear)
+				   struct splice_pipe_desc *spd, int linear,
+				   struct sock *sk)
 {
 	if (!*len)
 		return 1;
@@ -1465,7 +1466,7 @@ static inline int __splice_segment(struct page *page, unsigned int poff,
 		/* the linear region may spread across several pages  */
 		flen = min_t(unsigned int, flen, PAGE_SIZE - poff);
 
-		if (spd_fill_page(spd, page, &flen, poff, skb, linear))
+		if (spd_fill_page(spd, page, &flen, poff, skb, linear, sk))
 			return 1;
 
 		__segment_seek(&page, &poff, &plen, flen);
@@ -1481,8 +1482,8 @@ static inline int __splice_segment(struct page *page, unsigned int poff,
  * pipe is full or if we already spliced the requested length.
  */
 static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
-		      unsigned int *len,
-		      struct splice_pipe_desc *spd)
+			     unsigned int *len, struct splice_pipe_desc *spd,
+			     struct sock *sk)
 {
 	int seg;
 
@@ -1492,7 +1493,7 @@ static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
 	if (__splice_segment(virt_to_page(skb->data),
 			     (unsigned long) skb->data & (PAGE_SIZE - 1),
 			     skb_headlen(skb),
-			     offset, len, skb, spd, 1))
+			     offset, len, skb, spd, 1, sk))
 		return 1;
 
 	/*
@@ -1502,7 +1503,7 @@ static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
 		const skb_frag_t *f = &skb_shinfo(skb)->frags[seg];
 
 		if (__splice_segment(f->page, f->page_offset, f->size,
-				     offset, len, skb, spd, 0))
+				     offset, len, skb, spd, 0, sk))
 			return 1;
 	}
 
@@ -1528,12 +1529,13 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
 		.ops = &sock_pipe_buf_ops,
 		.spd_release = sock_spd_release,
 	};
+	struct sock *sk = skb->sk;
 
 	/*
 	 * __skb_splice_bits() only fails if the output has no room left,
 	 * so no point in going over the frag_list for the error case.
 	 */
-	if (__skb_splice_bits(skb, &offset, &tlen, &spd))
+	if (__skb_splice_bits(skb, &offset, &tlen, &spd, sk))
 		goto done;
 	else if (!tlen)
 		goto done;
@@ -1545,14 +1547,13 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
 		struct sk_buff *list = skb_shinfo(skb)->frag_list;
 
 		for (; list && tlen; list = list->next) {
-			if (__skb_splice_bits(list, &offset, &tlen, &spd))
+			if (__skb_splice_bits(list, &offset, &tlen, &spd, sk))
 				break;
 		}
 	}
 
 done:
 	if (spd.nr_pages) {
-		struct sock *sk = skb->sk;
 		int ret;
 
 		/*

[PATCH net-2.6] Re: oopses since "net: Optimize memory usage when splicing from sockets"

[Jarek Poplawski]

On Thu, Apr 30, 2009 at 05:16:26AM +0200, Lennert Buytenhek wrote:
> Since 4fb669948116d928ae44262ab7743732c574630d ("net: Optimize memory
> usage when splicing from sockets.") I'm seeing this oops (e.g. in
> 2.6.30-rc3) when splicing from a TCP socket to /dev/null on a driver
> (mv643xx_eth) that uses LRO in the skb mode (lro_receive_skb) rather
> than the frag mode:
...
> addr2line suggests skb->sk is NULL in linear_to_page():
> 
> 
> 	static inline struct page *linear_to_page(struct page *page, unsigned int *len,
> 						  unsigned int *offset,
> 						  struct sk_buff *skb)
> 	{
> 		struct sock *sk = skb->sk;
> 		struct page *p = sk->sk_sndmsg_page;		<========
> 		unsigned int off;
> 
> 		if (!p) {
> 
> 
> When we get here, skb->sk has apparently already been dropped, leading
> to a NULL pointer deref.  Backing out the offending commit makes the
> oops go away (as does converting the driver to lro frag rx, but that
> destroys routing performance).
> 
> Thoughts?  Should we just fall back to plain alloc_pages() if skb->sk
> is NULL, or should have still have the socket reference when we get here?

It seems my first impression could be quite right yet, so if there is
no better idea I think it should go to the mainline with Lennert's
"Tested-by", I hope.

Thanks,
Jarek P.
------------------------->
net: Fix oops when splicing skbs from a frag_list.

Lennert Buytenhek wrote:
> Since 4fb669948116d928ae44262ab7743732c574630d ("net: Optimize memory
> usage when splicing from sockets.") I'm seeing this oops (e.g. in
> 2.6.30-rc3) when splicing from a TCP socket to /dev/null on a driver
> (mv643xx_eth) that uses LRO in the skb mode (lro_receive_skb) rather
> than the frag mode:

My patch incorrectly assumed skb->sk was always valid, but for
"frag_listed" skbs we can only use skb->sk of their parent.

Reported-by: Lennert Buytenhek <buytenh@wantstofly.org>
Debugged-by: Lennert Buytenhek <buytenh@wantstofly.org>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 net/core/skbuff.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ce6356c..f091a5a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1365,9 +1365,8 @@ static void sock_spd_release(struct splice_pipe_desc *spd, unsigned int i)
 
 static inline struct page *linear_to_page(struct page *page, unsigned int *len,
 					  unsigned int *offset,
-					  struct sk_buff *skb)
+					  struct sk_buff *skb, struct sock *sk)
 {
-	struct sock *sk = skb->sk;
 	struct page *p = sk->sk_sndmsg_page;
 	unsigned int off;
 
@@ -1405,13 +1404,14 @@ new_page:
  */
 static inline int spd_fill_page(struct splice_pipe_desc *spd, struct page *page,
 				unsigned int *len, unsigned int offset,
-				struct sk_buff *skb, int linear)
+				struct sk_buff *skb, int linear,
+				struct sock *sk)
 {
 	if (unlikely(spd->nr_pages == PIPE_BUFFERS))
 		return 1;
 
 	if (linear) {
-		page = linear_to_page(page, len, &offset, skb);
+		page = linear_to_page(page, len, &offset, skb, sk);
 		if (!page)
 			return 1;
 	} else
@@ -1442,7 +1442,8 @@ static inline void __segment_seek(struct page **page, unsigned int *poff,
 static inline int __splice_segment(struct page *page, unsigned int poff,
 				   unsigned int plen, unsigned int *off,
 				   unsigned int *len, struct sk_buff *skb,
-				   struct splice_pipe_desc *spd, int linear)
+				   struct splice_pipe_desc *spd, int linear,
+				   struct sock *sk)
 {
 	if (!*len)
 		return 1;
@@ -1465,7 +1466,7 @@ static inline int __splice_segment(struct page *page, unsigned int poff,
 		/* the linear region may spread across several pages  */
 		flen = min_t(unsigned int, flen, PAGE_SIZE - poff);
 
-		if (spd_fill_page(spd, page, &flen, poff, skb, linear))
+		if (spd_fill_page(spd, page, &flen, poff, skb, linear, sk))
 			return 1;
 
 		__segment_seek(&page, &poff, &plen, flen);
@@ -1481,8 +1482,8 @@ static inline int __splice_segment(struct page *page, unsigned int poff,
  * pipe is full or if we already spliced the requested length.
  */
 static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
-		      unsigned int *len,
-		      struct splice_pipe_desc *spd)
+			     unsigned int *len, struct splice_pipe_desc *spd,
+			     struct sock *sk)
 {
 	int seg;
 
@@ -1492,7 +1493,7 @@ static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
 	if (__splice_segment(virt_to_page(skb->data),
 			     (unsigned long) skb->data & (PAGE_SIZE - 1),
 			     skb_headlen(skb),
-			     offset, len, skb, spd, 1))
+			     offset, len, skb, spd, 1, sk))
 		return 1;
 
 	/*
@@ -1502,7 +1503,7 @@ static int __skb_splice_bits(struct sk_buff *skb, unsigned int *offset,
 		const skb_frag_t *f = &skb_shinfo(skb)->frags[seg];
 
 		if (__splice_segment(f->page, f->page_offset, f->size,
-				     offset, len, skb, spd, 0))
+				     offset, len, skb, spd, 0, sk))
 			return 1;
 	}
 
@@ -1528,12 +1529,13 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
 		.ops = &sock_pipe_buf_ops,
 		.spd_release = sock_spd_release,
 	};
+	struct sock *sk = skb->sk;
 
 	/*
 	 * __skb_splice_bits() only fails if the output has no room left,
 	 * so no point in going over the frag_list for the error case.
 	 */
-	if (__skb_splice_bits(skb, &offset, &tlen, &spd))
+	if (__skb_splice_bits(skb, &offset, &tlen, &spd, sk))
 		goto done;
 	else if (!tlen)
 		goto done;
@@ -1545,14 +1547,13 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
 		struct sk_buff *list = skb_shinfo(skb)->frag_list;
 
 		for (; list && tlen; list = list->next) {
-			if (__skb_splice_bits(list, &offset, &tlen, &spd))
+			if (__skb_splice_bits(list, &offset, &tlen, &spd, sk))
 				break;
 		}
 	}
 
 done:
 	if (spd.nr_pages) {
-		struct sock *sk = skb->sk;
 		int ret;
 
 		/*

Re: [PATCH net-2.6] Re: oopses since "net: Optimize memory usage when splicing from sockets"

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 30 Apr 2009 14:30:57 +0200

> It seems my first impression could be quite right yet, so if there is
> no better idea I think it should go to the mainline with Lennert's
> "Tested-by", I hope.

Yes, once this is tested I will apply this.

Re: oopses since "net: Optimize memory usage when splicing from sockets"

[Lennert Buytenhek]

On Thu, Apr 30, 2009 at 10:43:21AM +0200, Jarek Poplawski wrote:

> > Since 4fb669948116d928ae44262ab7743732c574630d ("net: Optimize memory
> > usage when splicing from sockets.") I'm seeing this oops (e.g. in
> > 2.6.30-rc3) when splicing from a TCP socket to /dev/null on a driver
> > (mv643xx_eth) that uses LRO in the skb mode (lro_receive_skb) rather
> > than the frag mode:
> ...
> > addr2line suggests skb->sk is NULL in linear_to_page():
> > 
> > 
> > 	static inline struct page *linear_to_page(struct page *page, unsigned int *len,
> > 						  unsigned int *offset,
> > 						  struct sk_buff *skb)
> > 	{
> > 		struct sock *sk = skb->sk;
> > 		struct page *p = sk->sk_sndmsg_page;		<========
> > 		unsigned int off;
> > 
> > 		if (!p) {
> > 
> > 
> > When we get here, skb->sk has apparently already been dropped, leading
> > to a NULL pointer deref.  Backing out the offending commit makes the
> > oops go away (as does converting the driver to lro frag rx, but that
> > destroys routing performance).
> > 
> > Thoughts?  Should we just fall back to plain alloc_pages() if skb->sk
> > is NULL, or should have still have the socket reference when we get here?
> 
> Hmm... I definitely need more time for this, but the first and maybe
> wrong impression is this is an skb from the frag_list. There are
> probably better ways of fixing it properly, but here is a quick hack
> for the beginning (alas not even compile-tested at the moment).

With your patch, at least the oops is gone, and I guess it makes
sense and looks correct, so:

	Tested-by: Lennert Buytenhek <buytenh@wantstofly.org>

Thanks!

Re: oopses since "net: Optimize memory usage when splicing from sockets"

[David Miller]

From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Thu, 30 Apr 2009 14:40:36 +0200

> With your patch, at least the oops is gone, and I guess it makes
> sense and looks correct, so:
> 
> 	Tested-by: Lennert Buytenhek <buytenh@wantstofly.org>

Thanks for testing Lennert.

I'll queue this up for -stable too.

Re: oopses since "net: Optimize memory usage when splicing from sockets"

[David Miller]

From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Thu, 30 Apr 2009 14:45:42 +0200

> On Thu, Apr 30, 2009 at 05:40:40AM -0700, David Miller wrote:
> 
>> > With your patch, at least the oops is gone, and I guess it makes
>> > sense and looks correct, so:
>> > 
>> > 	Tested-by: Lennert Buytenhek <buytenh@wantstofly.org>
>> 
>> Thanks for testing Lennert.
>> 
>> I'll queue this up for -stable too.
> 
> I don't see this in 2.6.29, since the offending commit was merged
> after 2.6.29 AFAICS?

You're right, it's not needed there.  The optimization went into
2.6.30-rcX only.

Thanks!

Re: oopses since "net: Optimize memory usage when splicing from sockets"

[Lennert Buytenhek]

On Thu, Apr 30, 2009 at 05:40:40AM -0700, David Miller wrote:

> > With your patch, at least the oops is gone, and I guess it makes
> > sense and looks correct, so:
> > 
> > 	Tested-by: Lennert Buytenhek <buytenh@wantstofly.org>
> 
> Thanks for testing Lennert.
> 
> I'll queue this up for -stable too.

I don't see this in 2.6.29, since the offending commit was merged
after 2.6.29 AFAICS?

Re: oopses since "net: Optimize memory usage when splicing from sockets"

[Jarek Poplawski]

On Thu, Apr 30, 2009 at 02:45:42PM +0200, Lennert Buytenhek wrote:
> On Thu, Apr 30, 2009 at 05:40:40AM -0700, David Miller wrote:
...
> > Thanks for testing Lennert.
> > 
> > I'll queue this up for -stable too.
> 
> I don't see this in 2.6.29, since the offending commit was merged
> after 2.6.29 AFAICS?

Yes, David had a very good intuition some time ago, and opposed my
-stable recommendation!

Thanks for this,
Jarek P.