diff mbox

[RFT,v2] sh_eth: fix kernel oops in skb_put()

Message ID 2611049.bTOQ0T0Nsl@wasted.cogentembedded.com (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Sergei Shtylyov Oct. 24, 2015, 10:42 p.m. UTC 
In a low memory situation the following kernel oops occurs:

Unable to handle kernel NULL pointer dereference at virtual address 00000050
pgd = 8490c000
[00000050] *pgd=4651e831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT ARM
Modules linked in:
CPU: 0    Not tainted  (3.4-at16 #9)
PC is at skb_put+0x10/0x98
LR is at sh_eth_poll+0x2c8/0xa10
pc : [<8035f780>]    lr : [<8028bf50>]    psr: 60000113
sp : 84eb1a90  ip : 84eb1ac8  fp : 84eb1ac4
r10: 0000003f  r9 : 000005ea  r8 : 00000000
r7 : 00000000  r6 : 940453b0  r5 : 00030000  r4 : 9381b180
r3 : 00000000  r2 : 00000000  r1 : 000005ea  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 10c53c7d  Table: 4248c059  DAC: 00000015
Process klogd (pid: 2046, stack limit = 0x84eb02e8)
[...]

This is because netdev_alloc_skb() fails and 'mdp->rx_skbuff[entry]' is left
NULL but sh_eth_rx() later uses it without checking. Add such check...

Reported-by: Yasushi SHOJI <yashi@atmark-techno.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against DaveM's 'net.git' repo.

 drivers/net/ethernet/renesas/sh_eth.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


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

Comments

Yasushi SHOJI Oct. 26, 2015, 10:52 p.m. UTC | #1 
Hi Sergei,

Thank you for your patch!

On Sun, 25 Oct 2015 07:42:33 +0900,
Sergei Shtylyov wrote:
> 
> In a low memory situation the following kernel oops occurs:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000050
> pgd = 8490c000
> [00000050] *pgd=4651e831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 17 [#1] PREEMPT ARM
> Modules linked in:
> CPU: 0    Not tainted  (3.4-at16 #9)
> PC is at skb_put+0x10/0x98
> LR is at sh_eth_poll+0x2c8/0xa10
> pc : [<8035f780>]    lr : [<8028bf50>]    psr: 60000113
> sp : 84eb1a90  ip : 84eb1ac8  fp : 84eb1ac4
> r10: 0000003f  r9 : 000005ea  r8 : 00000000
> r7 : 00000000  r6 : 940453b0  r5 : 00030000  r4 : 9381b180
> r3 : 00000000  r2 : 00000000  r1 : 000005ea  r0 : 00000000
> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 10c53c7d  Table: 4248c059  DAC: 00000015
> Process klogd (pid: 2046, stack limit = 0x84eb02e8)
> [...]
> 
> This is because netdev_alloc_skb() fails and 'mdp->rx_skbuff[entry]' is left
> NULL but sh_eth_rx() later uses it without checking. Add such check...
> 
> Reported-by: Yasushi SHOJI <yashi@atmark-techno.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> This patch is against DaveM's 'net.git' repo.
> 
>  drivers/net/ethernet/renesas/sh_eth.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: net/drivers/net/ethernet/renesas/sh_eth.c
> ===================================================================
> --- net.orig/drivers/net/ethernet/renesas/sh_eth.c
> +++ net/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1481,6 +1481,7 @@ static int sh_eth_rx(struct net_device *
>  		if (mdp->cd->shift_rd0)
>  			desc_status >>= 16;
>  
> +		skb = mdp->rx_skbuff[entry];
>  		if (desc_status & (RD_RFS1 | RD_RFS2 | RD_RFS3 | RD_RFS4 |
>  				   RD_RFS5 | RD_RFS6 | RD_RFS10)) {
>  			ndev->stats.rx_errors++;
> @@ -1496,12 +1497,11 @@ static int sh_eth_rx(struct net_device *
>  				ndev->stats.rx_missed_errors++;
>  			if (desc_status & RD_RFS10)
>  				ndev->stats.rx_over_errors++;
> -		} else {
> +		} else	if (skb) {
>  			if (!mdp->cd->hw_swap)
>  				sh_eth_soft_swap(
>  					phys_to_virt(ALIGN(rxdesc->addr, 4)),
>  					pkt_len + 2);
> -			skb = mdp->rx_skbuff[entry];
>  			mdp->rx_skbuff[entry] = NULL;
>  			if (mdp->cd->rpadir)
>  				skb_reserve(skb, NET_IP_ALIGN);
> 

This certainly prevents from a bad access, however, some odd thing is
going on.  Once I hit a low memory situation with this patch, network
thorough-put and response is very bad.

telnet, ping, wget takes loong time.

PING 172.16.2.13 (172.16.2.13) 56(84) bytes of data.
64 bytes from 172.16.2.13: icmp_seq=5 ttl=64 time=0.223 ms
64 bytes from 172.16.2.13: icmp_seq=6 ttl=64 time=0.195 ms
64 bytes from 172.16.2.13: icmp_seq=7 ttl=64 time=0.203 ms
64 bytes from 172.16.2.13: icmp_seq=8 ttl=64 time=0.219 ms
64 bytes from 172.16.2.13: icmp_seq=9 ttl=64 time=0.165 ms
64 bytes from 172.16.2.13: icmp_seq=10 ttl=64 time=0.171 ms
64 bytes from 172.16.2.13: icmp_seq=1 ttl=64 time=9023 ms
64 bytes from 172.16.2.13: icmp_seq=2 ttl=64 time=8022 ms
64 bytes from 172.16.2.13: icmp_seq=3 ttl=64 time=7014 ms
64 bytes from 172.16.2.13: icmp_seq=4 ttl=64 time=6006 ms

I'll investigate it.
David Miller Oct. 30, 2015, 9:02 a.m. UTC | #2 
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sun, 25 Oct 2015 01:42:33 +0300

> In a low memory situation the following kernel oops occurs:
 ...
> This is because netdev_alloc_skb() fails and 'mdp->rx_skbuff[entry]' is left
> NULL but sh_eth_rx() later uses it without checking. Add such check...
> 
> Reported-by: Yasushi SHOJI <yashi@atmark-techno.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

This seems to need more investigation and work, therefore I am
not applying this at this time.

When a final version is available (or even if this one is deemed
appropriate as-is) please resubmit it withut RFT in the Subject.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Nov. 19, 2015, 5:53 p.m. UTC | #3 
Hello.

On 10/27/2015 01:52 AM, Yasushi SHOJI wrote:

>> In a low memory situation the following kernel oops occurs:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 00000050
>> pgd = 8490c000
>> [00000050] *pgd=4651e831, *pte=00000000, *ppte=00000000
>> Internal error: Oops: 17 [#1] PREEMPT ARM
>> Modules linked in:
>> CPU: 0    Not tainted  (3.4-at16 #9)
>> PC is at skb_put+0x10/0x98
>> LR is at sh_eth_poll+0x2c8/0xa10
>> pc : [<8035f780>]    lr : [<8028bf50>]    psr: 60000113
>> sp : 84eb1a90  ip : 84eb1ac8  fp : 84eb1ac4
>> r10: 0000003f  r9 : 000005ea  r8 : 00000000
>> r7 : 00000000  r6 : 940453b0  r5 : 00030000  r4 : 9381b180
>> r3 : 00000000  r2 : 00000000  r1 : 000005ea  r0 : 00000000
>> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
>> Control: 10c53c7d  Table: 4248c059  DAC: 00000015
>> Process klogd (pid: 2046, stack limit = 0x84eb02e8)
>> [...]
>>
>> This is because netdev_alloc_skb() fails and 'mdp->rx_skbuff[entry]' is left
>> NULL but sh_eth_rx() later uses it without checking. Add such check...
>>
>> Reported-by: Yasushi SHOJI <yashi@atmark-techno.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> This patch is against DaveM's 'net.git' repo.
>>
>>   drivers/net/ethernet/renesas/sh_eth.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Index: net/drivers/net/ethernet/renesas/sh_eth.c
>> ===================================================================
>> --- net.orig/drivers/net/ethernet/renesas/sh_eth.c
>> +++ net/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -1481,6 +1481,7 @@ static int sh_eth_rx(struct net_device *
>>   		if (mdp->cd->shift_rd0)
>>   			desc_status >>= 16;
>>
>> +		skb = mdp->rx_skbuff[entry];
>>   		if (desc_status & (RD_RFS1 | RD_RFS2 | RD_RFS3 | RD_RFS4 |
>>   				   RD_RFS5 | RD_RFS6 | RD_RFS10)) {
>>   			ndev->stats.rx_errors++;
>> @@ -1496,12 +1497,11 @@ static int sh_eth_rx(struct net_device *
>>   				ndev->stats.rx_missed_errors++;
>>   			if (desc_status & RD_RFS10)
>>   				ndev->stats.rx_over_errors++;
>> -		} else {
>> +		} else	if (skb) {
>>   			if (!mdp->cd->hw_swap)
>>   				sh_eth_soft_swap(
>>   					phys_to_virt(ALIGN(rxdesc->addr, 4)),
>>   					pkt_len + 2);
>> -			skb = mdp->rx_skbuff[entry];
>>   			mdp->rx_skbuff[entry] = NULL;
>>   			if (mdp->cd->rpadir)
>>   				skb_reserve(skb, NET_IP_ALIGN);
>>
>
> This certainly prevents from a bad access, however, some odd thing is
> going on.  Once I hit a low memory situation with this patch, network
> thorough-put and response is very bad.

> telnet, ping, wget takes loong time.
>
> PING 172.16.2.13 (172.16.2.13) 56(84) bytes of data.
> 64 bytes from 172.16.2.13: icmp_seq=5 ttl=64 time=0.223 ms
> 64 bytes from 172.16.2.13: icmp_seq=6 ttl=64 time=0.195 ms
> 64 bytes from 172.16.2.13: icmp_seq=7 ttl=64 time=0.203 ms
> 64 bytes from 172.16.2.13: icmp_seq=8 ttl=64 time=0.219 ms
> 64 bytes from 172.16.2.13: icmp_seq=9 ttl=64 time=0.165 ms
> 64 bytes from 172.16.2.13: icmp_seq=10 ttl=64 time=0.171 ms
> 64 bytes from 172.16.2.13: icmp_seq=1 ttl=64 time=9023 ms
> 64 bytes from 172.16.2.13: icmp_seq=2 ttl=64 time=8022 ms
> 64 bytes from 172.16.2.13: icmp_seq=3 ttl=64 time=7014 ms
> 64 bytes from 172.16.2.13: icmp_seq=4 ttl=64 time=6006 ms
>
> I'll investigate it.

    Shoji-san, can I push this patch to net.git? I doubt that it has ill 
effects in itself -- the reason of the slowdown you're seeing should be 
somewhere else...

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yasushi SHOJI Nov. 20, 2015, 11:18 a.m. UTC | #4 
Hi Sergei,

On Fri, 20 Nov 2015 02:53:39 +0900,
Sergei Shtylyov wrote:
> 
>    Shoji-san, can I push this patch to net.git? I doubt that it has
> ill effects in itself -- the reason of the slowdown you're seeing
> should be somewhere else...

Sure.  I've tested and the null access problem is gone for sure.  I'm
pretty sure that the fix won't break anything.

It's going to take, however, some more time to pin down the slow down
problem.  I'll report when I find the cause.

Thanks,
diff mbox

Patch

Index: net/drivers/net/ethernet/renesas/sh_eth.c
===================================================================
--- net.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net/drivers/net/ethernet/renesas/sh_eth.c
@@ -1481,6 +1481,7 @@  static int sh_eth_rx(struct net_device *
 		if (mdp->cd->shift_rd0)
 			desc_status >>= 16;
 
+		skb = mdp->rx_skbuff[entry];
 		if (desc_status & (RD_RFS1 | RD_RFS2 | RD_RFS3 | RD_RFS4 |
 				   RD_RFS5 | RD_RFS6 | RD_RFS10)) {
 			ndev->stats.rx_errors++;
@@ -1496,12 +1497,11 @@  static int sh_eth_rx(struct net_device *
 				ndev->stats.rx_missed_errors++;
 			if (desc_status & RD_RFS10)
 				ndev->stats.rx_over_errors++;
-		} else {
+		} else	if (skb) {
 			if (!mdp->cd->hw_swap)
 				sh_eth_soft_swap(
 					phys_to_virt(ALIGN(rxdesc->addr, 4)),
 					pkt_len + 2);
-			skb = mdp->rx_skbuff[entry];
 			mdp->rx_skbuff[entry] = NULL;
 			if (mdp->cd->rpadir)
 				skb_reserve(skb, NET_IP_ALIGN);