RE: NULL Pointer Deference: NFS & Telnet

RE: NULL Pointer Deference: NFS & Telnet

[Arce, Abraham]

Hi,

I am able to avoid the NULL pointer dereference but not sure if the handling
is the correct one... find the patch below...

> I have 2 scenarios in which I am getting a NULL pointer dereference:
> 
> 1) root filesystem over nfs
> 2) telnet connection
> 
> The issue appeared on this commit
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-
> 2.6.git;a=commit;h=f8965467f366fd18f01feafb5db10512d7b4422c
> 
> The driver I am working with is drivers/net/ks8851.c
> Any help will be highly appreciated...
>
> ---
> 
> Scenario 1 | root filesystem over nfs
> 
> Looking up port of RPC 100005/1 on 10.87.231.229
> VFS: Mounted root (nfs filesystem) on device 0:10.
> Freeing init memory: 128K
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [..]
> PC is at put_page+0xc/0x120
> LR is at skb_release_data+0x74/0xb8
> [..]
> Backtrace:
> [<c0086dd0>] (put_page+0x0/0x120)
> [<c01d0a48>] (skb_release_data+0x0/0xb8)
> [<c01d1044>] (skb_release_all+0x0/0x20)
> [<c01d075c>] (__kfree_skb+0x0/0xbc)
> [<c01d0818>] (consume_skb+0x0/0x58)
> [<c01d39cc>] (skb_free_datagram+0x0/0x40)
> [<c023ff74>] (xs_udp_data_ready+0x0/0x1e8)
> [<c01ce034>] (sock_queue_rcv_skb+0x0/0x1c0)
> [<c01fbba8>] (ip_queue_rcv_skb+0x0/0x58)
> [<c02176c0>] (__udp_queue_rcv_skb+0x0/0x18c)
> [<c0218e28>] (udp_queue_rcv_skb+0x0/0x348)
> [<c02195a4>] (__udp4_lib_rcv+0x0/0x564)
> [<c0219b08>] (udp_rcv+0x0/0x20)
> [<c01f5f34>] (ip_local_deliver+0x0/0x264)
> [<c01f586c>] (ip_rcv+0x0/0x6c8)
> [<c01d7ec0>] (__netif_receive_skb+0x0/0x2d0)
> [<c01d8190>] (process_backlog+0x0/0x16c)
> [<c01d8e14>] (net_rx_action+0x0/0x18c)
> [<c00521a0>] (__do_softirq+0x0/0x12c)
> [<c00522cc>] (irq_exit+0x0/0x70)
> [<c0028000>] (asm_do_IRQ+0x0/0xc8)
> 
> Complete log at http://pastebin.mozilla.org/728027
> 
> ---
> 
> Scenario 2
> 
>  1. Root filesystem booted in ram
>  2. eth0 brought up
>  3. telnetd daemon started
>  4. tried to connect through telnet
> 
> # Unable to handle kernel NULL pointer dereference at virtual address 00000000
> pgd = d98e8000
> [..]
> PC is at put_page+0xc/0x120
> LR is at skb_release_data+0x74/0xb8
> [..]
> Backtrace:
> [<c0086dd0>] (put_page+0x0/0x120)
> [<c01d0a48>] (skb_release_data+0x0/0xb8)
> [<c01d1044>] (skb_release_all+0x0/0x20)
> [<c01d075c>] (__kfree_skb+0x0/0xbc)
> [<c0202444>] (tcp_recvmsg+0x0/0x93c)
> [<c02201e8>] (inet_recvmsg+0x0/0xec)
> [<c01c7fd0>] (sock_aio_read+0x0/0xf8)
> [<c00ab3ac>] (do_sync_read+0x0/0xec)
> [<c00abfbc>] (vfs_read+0x0/0x164)
> [<c00ac1a0>] (sys_read+0x0/0x70)
> [<c0029100>] (ret_fast_syscall+0x0/0x30)
> 
> Complete log at http://pastebin.mozilla.org/728028
> 

Check for NULL data in sk_buff before sending to put_page

Signed-off-by: Abraham Arce <x0066660-l0cyMroinI0@public.gmane.org>
---
 net/core/skbuff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f8abf68..eb81f76 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -334,7 +334,7 @@ static void skb_release_data(struct sk_buff *skb)
 	if (!skb->cloned ||
 	    !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
 			       &skb_shinfo(skb)->dataref)) {
-		if (skb_shinfo(skb)->nr_frags) {
+		if (skb_shinfo(skb)->nr_frags && skb_has_frags(skb)) {
 			int i;
 			for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
 				put_page(skb_shinfo(skb)->frags[i].page);
-- 
1.5.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NULL Pointer Deference: NFS & Telnet

[David Miller]

From: "Arce, Abraham" <x0066660-l0cyMroinI0@public.gmane.org>
Date: Tue, 25 May 2010 20:48:02 -0500

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f8abf68..eb81f76 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -334,7 +334,7 @@ static void skb_release_data(struct sk_buff *skb)
>  	if (!skb->cloned ||
>  	    !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
>  			       &skb_shinfo(skb)->dataref)) {
> -		if (skb_shinfo(skb)->nr_frags) {
> +		if (skb_shinfo(skb)->nr_frags && skb_has_frags(skb)) {
>  			int i;
>  			for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
>  				put_page(skb_shinfo(skb)->frags[i].page);

skb_shinfo(skb)->nr_frags counts the number of entries contained
in the skb_shinfo(skb)->frags[] array.

This has nothing to do with the frag list pointer,
skb_shinfo(skb)->frag_list, which is what skb_has_frags()
tests.

You've got some kind of memory corruption going on and it
appears to have nothing to do with the code paths you're
playing with here.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: NULL Pointer Deference: NFS & Telnet

[Arce, Abraham]

Thanks David,

> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index f8abf68..eb81f76 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -334,7 +334,7 @@ static void skb_release_data(struct sk_buff *skb)
> >  	if (!skb->cloned ||
> >  	    !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
> >  			       &skb_shinfo(skb)->dataref)) {
> > -		if (skb_shinfo(skb)->nr_frags) {
> > +		if (skb_shinfo(skb)->nr_frags && skb_has_frags(skb)) {
> >  			int i;
> >  			for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> >  				put_page(skb_shinfo(skb)->frags[i].page);
> 
> skb_shinfo(skb)->nr_frags counts the number of entries contained
> in the skb_shinfo(skb)->frags[] array.
> 
> This has nothing to do with the frag list pointer,
> skb_shinfo(skb)->frag_list, which is what skb_has_frags()
> tests.
> 
> You've got some kind of memory corruption going on and it
> appears to have nothing to do with the code paths you're
> playing with here.

Do you have any recommendation on debugging technique/tool for this memory corruption issue?

Best Regards
Abraham
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: NULL Pointer Deference: NFS & Telnet

[Eric Dumazet]

Le mardi 25 mai 2010 à 21:02 -0500, Arce, Abraham a écrit :
> Thanks David,
> 
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index f8abf68..eb81f76 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > > @@ -334,7 +334,7 @@ static void skb_release_data(struct sk_buff *skb)
> > >  	if (!skb->cloned ||
> > >  	    !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
> > >  			       &skb_shinfo(skb)->dataref)) {
> > > -		if (skb_shinfo(skb)->nr_frags) {
> > > +		if (skb_shinfo(skb)->nr_frags && skb_has_frags(skb)) {
> > >  			int i;
> > >  			for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> > >  				put_page(skb_shinfo(skb)->frags[i].page);
> > 
> > skb_shinfo(skb)->nr_frags counts the number of entries contained
> > in the skb_shinfo(skb)->frags[] array.
> > 
> > This has nothing to do with the frag list pointer,
> > skb_shinfo(skb)->frag_list, which is what skb_has_frags()
> > tests.
> > 
> > You've got some kind of memory corruption going on and it
> > appears to have nothing to do with the code paths you're
> > playing with here.
> 
> Do you have any recommendation on debugging technique/tool for this memory corruption issue?
> 
> Best Regards
> Abraham
> --

It seems quite strange. You have a skb->nr_frags > 0 value, but a
frags[i].page = 0 value

You might add following function :

shinfo_check(struct sk_buff *skb)
{
	struct skb_shared_info *shinfo = skb_shinfo(skb);
	int i;

	WARN_ON(shinfo->nr_frags >= MAX_SKB_FRAGS);
	for (i = 0; i < shinfo->nr_frags; i++)
		WARN_ON(!shinfo->frags[i].page);
}

And call it from various points, to check who corrupts your skb.



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: NULL Pointer Deference: NFS & Telnet

[Arce, Abraham]

Thanks Eric, David,

[..]

> > > > -		if (skb_shinfo(skb)->nr_frags) {
> > > > +		if (skb_shinfo(skb)->nr_frags && skb_has_frags(skb)) {
> > > >  			int i;
> > > >  			for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> > > >  				put_page(skb_shinfo(skb)->frags[i].page);
> > >
> > > skb_shinfo(skb)->nr_frags counts the number of entries contained
> > > in the skb_shinfo(skb)->frags[] array.
> > >
> > > This has nothing to do with the frag list pointer,
> > > skb_shinfo(skb)->frag_list, which is what skb_has_frags()
> > > tests.
> > >
> > > You've got some kind of memory corruption going on and it
> > > appears to have nothing to do with the code paths you're
> > > playing with here.
> >
> > Do you have any recommendation on debugging technique/tool for this memory
> corruption issue?

[..]

> It seems quite strange. You have a skb->nr_frags > 0 value, but a
> frags[i].page = 0 value
> 
> You might add following function :
> 
> shinfo_check(struct sk_buff *skb)
> {
> 	struct skb_shared_info *shinfo = skb_shinfo(skb);
> 	int i;
> 
> 	WARN_ON(shinfo->nr_frags >= MAX_SKB_FRAGS);
> 	for (i = 0; i < shinfo->nr_frags; i++)
> 		WARN_ON(!shinfo->frags[i].page);
> }
> 
> And call it from various points, to check who corrupts your skb.

By increasing the allocation length of our rx skbuff the corruption issue is fixed... I have increased it by 2... Were we writing outside our boundaries of skb data?

Please let me know about this approach...

diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
index b4fb07a..6da81e1 100644
--- a/drivers/net/ks8851.c
+++ b/drivers/net/ks8851.c
@@ -504,7 +504,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
                               ks->rc_rxqcr | RXQCR_SDA | RXQCR_ADRFE);

                if (rxlen > 0) {
-                       skb = netdev_alloc_skb(ks->netdev, rxlen + 2 + 8);
+                       skb = netdev_alloc_skb(ks->netdev, rxlen + 4 + 8);
                        if (!skb) {

Best Regards
Abraham

RE: NULL Pointer Deference: NFS & Telnet

[Eric Dumazet]

Le mercredi 26 mai 2010 à 15:19 -0500, Arce, Abraham a écrit :

> By increasing the allocation length of our rx skbuff the corruption issue is fixed... I have increased it by 2... Were we writing outside our boundaries of skb data?
> 
> Please let me know about this approach...
> 
> diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
> index b4fb07a..6da81e1 100644
> --- a/drivers/net/ks8851.c
> +++ b/drivers/net/ks8851.c
> @@ -504,7 +504,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>                                ks->rc_rxqcr | RXQCR_SDA | RXQCR_ADRFE);
> 
>                 if (rxlen > 0) {
> -                       skb = netdev_alloc_skb(ks->netdev, rxlen + 2 + 8);
> +                       skb = netdev_alloc_skb(ks->netdev, rxlen + 4 + 8);
>                         if (!skb) {
> 
> Best Regards
> Abraham
> 

Yes that makes sense, nr_frag is right after the packet (padded to L1
cache size)

But please do the correct allocation ?

Also, we dont need FCS ?

diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
index b4fb07a..05bd312 100644
--- a/drivers/net/ks8851.c
+++ b/drivers/net/ks8851.c
@@ -503,8 +503,9 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 		ks8851_wrreg16(ks, KS_RXQCR,
 			       ks->rc_rxqcr | RXQCR_SDA | RXQCR_ADRFE);
 
-		if (rxlen > 0) {
-			skb = netdev_alloc_skb(ks->netdev, rxlen + 2 + 8);
+		if (rxlen > 4) {
+			rxlen -= 4;
+			skb = netdev_alloc_skb(ks->netdev, 2 + 8 + ALIGN(rxlen, 4));
 			if (!skb) {
 				/* todo - dump frame and move on */
 			}
@@ -513,7 +514,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 			 * for the status header and 4 bytes of garbage */
 			skb_reserve(skb, 2 + 4 + 4);
 
-			rxpkt = skb_put(skb, rxlen - 4) - 8;
+			rxpkt = skb_put(skb, rxlen) - 8;
 
 			/* align the packet length to 4 bytes, and add 4 bytes
 			 * as we're getting the rx status header as well */
@@ -526,7 +527,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
 			netif_rx(skb);
 
 			ks->netdev->stats.rx_packets++;
-			ks->netdev->stats.rx_bytes += rxlen - 4;
+			ks->netdev->stats.rx_bytes += rxlen;
 		}
 
 		ks8851_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr);

Re: NULL Pointer Deference: NFS & Telnet

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 May 2010 22:48:53 +0200

> Le mercredi 26 mai 2010 à 15:19 -0500, Arce, Abraham a écrit :
> 
>> By increasing the allocation length of our rx skbuff the corruption issue is fixed... I have increased it by 2... Were we writing outside our boundaries of skb data?
>> 
>> Please let me know about this approach...
>> 
>> diff --git a/drivers/net/ks8851.c b/drivers/net/ks8851.c
>> index b4fb07a..6da81e1 100644
>> --- a/drivers/net/ks8851.c
>> +++ b/drivers/net/ks8851.c
>> @@ -504,7 +504,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>>                                ks->rc_rxqcr | RXQCR_SDA | RXQCR_ADRFE);
>> 
>>                 if (rxlen > 0) {
>> -                       skb = netdev_alloc_skb(ks->netdev, rxlen + 2 + 8);
>> +                       skb = netdev_alloc_skb(ks->netdev, rxlen + 4 + 8);
>>                         if (!skb) {
>> 
>> Best Regards
>> Abraham
>> 
> 
> Yes that makes sense, nr_frag is right after the packet (padded to L1
> cache size)
> 
> But please do the correct allocation ?
> 
> Also, we dont need FCS ?

Can we make some progress and get this patch tested and formally
submitted so we can kill this bug?

Thanks!