Skip to contentSkip to conversation

New: etting MTU > 1500 on card "cold" sigsegs the "ip" program and produces an OOPS.

Bugme-new Applied Andrew Morton 6 participants 17 messages Nov 15, 2007 – Dec 1, 2007 1 message not in the archive

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author
#1 lore raw
Quoted 91 lines · On Wed, 14 Nov 2007 19:11:38 -0800 (PST) [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=9382

Summary: etting MTU > 1500 on card "cold" sigsegs the "ip"
program and produces an OOPS.
Product: Drivers
Version: 2.5
KernelVersion: VIA Velocity > 1500 MTU cold configure -=> OOPS
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Network
AssignedTo: [email protected]
ReportedBy: [email protected]

Most recent kernel where this bug did not occur: Still occurs on latest openSUSE 10.3 kernel. A brief scan of the git log for this driver does not suggest a fix.

Distribution: openSUSE 10.3
Hardware Environment: AMD Athlon XP 2200
Software Environment: kernel 2.6.22.12
Problem Description: setting MTU > 1500 on card "cold" sigsegs the "ip" program and produces an OOPS.

Steps to reproduce:

1. have a VIA Velocity gig-e NIC, *cold* (unconfigured but module loaded).
2. Issue:

ip link set eth0 up 7200 192.168.1.1

This has been an issue since at least 2.6.18 but I wasn't able to reproduce it on demand for a whole variety of reasons. Now I am able to reproduce it, 100% of the time, on-demand (although this requires a reboot.)

BUG: unable to handle kernel NULL pointer dereference at virtual address

00000003
printing eip:
f96a7772
*pde = 00000000

Oops: 0000 [#1]
SMP
last sysfs file: /devices/pci0000:00/0000:00:00.0/class
Modules linked in: xt_tcpudp xt_pkttype ipt_LOG xt_limit drbd snd_pcm_oss

snd_mixer_oss snd_seq snd_seq_device ipt_REJECT xt_state iptable_mangle iptable_nat nf_nat iptable_filter nf_conntrack_ipv4 nf_conntrack nfnetlink ip_tables ip6_tables x_tables tcp_bic apparmor dm_crypt loop dm_mirror dm_log dm_mod snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm button parport_pc rtc_cmos via_velocity sr_mod cdrom snd_timer rtc_core parport snd crc_ccitt rtc_lib soundcore snd_page_alloc shpchp sis_agp pci_hotplug agpgart i2c_sis630 i2c_sis96x i2c_core sg usbhid hid ff_memless ehci_hcd sd_mod ohci_hcd usbcore piix sis5513 ide_core edd ext3 mbcache jbd fan pata_sis libata scsi_mod thermal processor

CPU:    0
EIP:    0060:[<f96a7772>]    Tainted: G      N VLI
EFLAGS: 00010046   (2.6.22.12-0.1-default #1)

EIP is at velocity_rx_refill+0x22/0x1d1 [via_velocity]
eax: c197a500 ebx: c197a500 ecx: 00000000 edx: df8a9c00
esi: c197a500 edi: 00000000 ebp: 00000000 esp: f774fe84
ds: 007b es: 007b fs: 00d8 gs: 0033 ss: 0068
Process ip (pid: 4133, ti=f774e000 task=f7a31030 task.ti=f774e000)

Stack: f774fef0 00000086 3f257025 c170a2a0 00000000 fffffff4 00000200 c197a500
       df8a9e00 fffffff4 f96a798f c01d0360 f96a61a7 c197a500 c197a000 00001c20
       00000000 f96a8955 00008922 00008922 00000216 c197a000 00000001 00008922
Call Trace:
 [<f96a798f>] velocity_init_rd_ring+0x6e/0xa0 [via_velocity]
 [<c01d0360>] __delay+0x6/0x7
 [<f96a61a7>] safe_disable_mii_autopoll+0x14/0x26 [via_velocity]
 [<f96a8955>] velocity_change_mtu+0xbc/0x104 [via_velocity]
 [<c026e62a>] dev_set_mtu+0x2d/0x5d
 [<c0270788>] dev_ioctl+0x3b1/0x43e
 [<c012daa8>] ptrace_notify+0x6a/0x94
 [<c026554b>] sock_ioctl+0x19f/0x1be
 [<c02653ac>] sock_ioctl+0x0/0x1be
 [<c017a131>] do_ioctl+0x21/0xa0
 [<c014dcc0>] audit_syscall_entry+0x105/0x13e
 [<c017a3e7>] vfs_ioctl+0x237/0x249
 [<c017a445>] sys_ioctl+0x4c/0x67
 [<c0104ea2>] syscall_call+0x7/0xb
 =======================
Code: ff 31 c0 5a 5b 5e 5f 5d c3 55 57 56 53 89 c3 83 ec 18 8b a8 f8 00 00 00

c7 44 24 10 00 00 00 00 89 ef c1 e7 04 03 bb 00 01 00 00 <80> 7f 03 00 0f 88 1d 01 00 00 8d 34 ed 00 00 00 00 03 b3 04 01
EIP: [<f96a7772>] velocity_rx_refill+0x22/0x1d1 [via_velocity] SS:ESP 0068:f774fe84

#4 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

In reply to Stephen Hemminger
Quoted 9 lines · Stephen Hemminger

Simple mtu change when device is down.
Fix http://bugzilla.kernel.org/show_bug.cgi?id=9382.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/via-velocity.c static int velocity_change_mtu(struct ne · line 1972

Quoted 10 lines
drivers/net/via-velocity.c +5 -0
@@ -1963,6 +1963,11 @@ static int velocity_change_mtu(struct ne
 		return -EINVAL;
 	}
 
+	if (!netif_running(dev)) {
+		dev->mtu = new_mtu;
+		return 0;
+	}
+
 	if (new_mtu != oldmtu) {
 		spin_lock_irqsave(&vptr->lock, flags);

Shouldn't this latter 'if' be removed now, btw?

Regards,
Jarek P.

#5 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

Quoted 13 lines · On Thu, 15 Nov 2007 09:26:00 +0100 Jarek Poplawski <[email protected]> wrote:
Quoted 12 lines · Stephen Hemminger

Simple mtu change when device is down.
Fix http://bugzilla.kernel.org/show_bug.cgi?id=9382.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/via-velocity.c +0 -0
@@ -1963,6 +1963,11 @@ static int velocity_change_mtu(struct ne
 		return -EINVAL;
 	}
Quoted 9 lines

drivers/net/via-velocity.c static int velocity_change_mtu(struct ne · line 1972

Quoted 7 lines
drivers/net/via-velocity.c +5 -0
+	if (!netif_running(dev)) {
+		dev->mtu = new_mtu;
+		return 0;
+	}
+
 	if (new_mtu != oldmtu) {
 		spin_lock_irqsave(&vptr->lock, flags);

Shouldn't this latter 'if' be removed now, btw?

No, it makes sense that if mtu is same, no action need be taken.

Actually, it would make sense to push the same check up into the netdevice core management.

Signature · 1 line
Stephen Hemminger <[email protected]>
#6 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

Stephen Hemminger wrote, On 11/15/2007 07:20 PM:

Quoted 25 lines
Quoted 13 lines · On Thu, 15 Nov 2007 09:26:00 +0100 Jarek Poplawski <[email protected]> wrote:
Quoted 12 lines · Stephen Hemminger

Simple mtu change when device is down.
Fix http://bugzilla.kernel.org/show_bug.cgi?id=9382.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/via-velocity.c +0 -0
@@ -1963,6 +1963,11 @@ static int velocity_change_mtu(struct ne
 		return -EINVAL;
 	}
Quoted 8 lines

drivers/net/via-velocity.c static int velocity_change_mtu(struct ne · line 1972

Quoted 7 lines
drivers/net/via-velocity.c +5 -0
+	if (!netif_running(dev)) {
+		dev->mtu = new_mtu;
+		return 0;
+	}
+
 	if (new_mtu != oldmtu) {
 		spin_lock_irqsave(&vptr->lock, flags);

Shouldn't this latter 'if' be removed now, btw?

Quoted 4 lines

No, it makes sense that if mtu is same, no action need be taken.

Actually, it would make sense to push the same check up into the netdevice core management.

Sure! I was only worried velocity_open() treats dev->mtu a bit different than velocity_change_mtu(), so eg. after:

velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up

with the same mtu, vptr->rx_buf_sz could be different than after:

velocity_open()
velocity_change_mtu() // dev is up

But, probably, I miss someting.

Thanks,
Jarek P.

#7 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

On 11/15/07, Jarek Poplawski <[email protected]> wrote:
..

Quoted 7 lines

Sure! I was only worried velocity_open() treats dev->mtu a bit different than velocity_change_mtu(), so eg. after:

velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up

Quoted 6 lines

with the same mtu, vptr->rx_buf_sz could be different than after:

velocity_open()
velocity_change_mtu() // dev is up

But, probably, I miss someting.

There is a snag here: if I change the MTU after the device is UP something ends up rather broken. A tcpdump shows nearly every outgoing frame has a bad TCP checksum (and the card does not support H/W checksumming or it is turned off as reported by ethtool).

Signature · 1 line
Jon
#8 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

Quoted 15 lines · On Thu, 15 Nov 2007 20:42:55 -0600 "Jon Nelson" <[email protected]> wrote:

On 11/15/07, Jarek Poplawski <[email protected]> wrote:
..

Quoted 12 lines

Sure! I was only worried velocity_open() treats dev->mtu a bit different than velocity_change_mtu(), so eg. after:

velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up

with the same mtu, vptr->rx_buf_sz could be different than after:

velocity_open()
velocity_change_mtu() // dev is up

Quoted 7 lines
Quoted 1 line

But, probably, I miss someting.

There is a snag here: if I change the MTU after the device is UP something ends up rather broken. A tcpdump shows nearly every outgoing frame has a bad TCP checksum (and the card does not support H/W checksumming or it is turned off as reported by ethtool).

That is a different (and pre-existing bug).

Signature · 1 line
Stephen Hemminger <[email protected]>
#9 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

In reply to Jon Nelson
Quoted 15 lines · On Thu, 15 Nov 2007 20:42:55 -0600 "Jon Nelson" <[email protected]> wrote:

On 11/15/07, Jarek Poplawski <[email protected]> wrote:
..

Quoted 12 lines

Sure! I was only worried velocity_open() treats dev->mtu a bit different than velocity_change_mtu(), so eg. after:

velocity_change_mtu() // dev is down
velocity_open()
velocity_change_mtu() // dev is up

with the same mtu, vptr->rx_buf_sz could be different than after:

velocity_open()
velocity_change_mtu() // dev is up

Quoted 7 lines
Quoted 1 line

But, probably, I miss someting.

There is a snag here: if I change the MTU after the device is UP something ends up rather broken. A tcpdump shows nearly every outgoing frame has a bad TCP checksum (and the card does not support H/W checksumming or it is turned off as reported by ethtool).

Does this fix the problem.

Note: reading the code the driver has other problems (besides crappy style). It does pci_map_single()/unmap in a way that doesn't account correctly for the padding that was added.

drivers/net/via-velocity.c +3 -8
@@ -1242,6 +1242,9 @@ static int velocity_rx_refill(struct vel
 static int velocity_init_rd_ring(struct velocity_info *vptr)
 {
 	int ret;
+	int mtu = vptr->dev->mtu;
+
+	vptr->rx_buf_sz = (mtu <= ETH_DATA_LEN) ? PKT_BUF_SZ : mtu + 32;
 
 	vptr->rd_info = kcalloc(vptr->options.numrx,
 				sizeof(struct velocity_rd_info), GFP_KERNEL);

@@ -1898,8 +1901,6 @@ static int velocity_open(struct net_devi
 	struct velocity_info *vptr = netdev_priv(dev);
 	int ret;
 
-	vptr->rx_buf_sz = (dev->mtu <= 1504 ? PKT_BUF_SZ : dev->mtu + 32);
-
 	ret = velocity_init_rings(vptr);
 	if (ret < 0)
 		goto out;

@@ -1978,12 +1979,6 @@ static int velocity_change_mtu(struct ne
 		velocity_free_rd_ring(vptr);
 
 		dev->mtu = new_mtu;
-		if (new_mtu > 8192)
-			vptr->rx_buf_sz = 9 * 1024;
-		else if (new_mtu > 4096)
-			vptr->rx_buf_sz = 8192;
-		else
-			vptr->rx_buf_sz = 4 * 1024;
 
 		ret = velocity_init_rd_ring(vptr);
 		if (ret < 0)
Signature · 1 line
Stephen Hemminger <[email protected]>
#11 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

Quoted 5 lines · On Fri, 16 Nov 2007 10:21:25 -0600 "Jon Nelson" <[email protected]> wrote:

I get a segmentation fault.

Should this patch be in addition to the one from bug 9382?
http://bugzilla.kernel.org/attachment.cgi?id=13555&action=view

Yes. The new patch adds just recomputes the rx buf size in one place, so it can be set when device is down.
Also, the computation will always yield same result now, the old code computed different values depending on whether it was initial or changed MTU.

Signature · 1 line
Stephen Hemminger <[email protected]>
maintainer
#12 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

From: Stephen Hemminger <[email protected]>
Date: Fri, 16 Nov 2007 08:47:06 -0800

Quoted 5 lines
Quoted 2 lines · On Fri, 16 Nov 2007 10:21:25 -0600 "Jon Nelson" <[email protected]> wrote:

I get a segmentation fault.

Quoted 8 lines
Quoted 3 lines

Should this patch be in addition to the one from bug 9382?
http://bugzilla.kernel.org/attachment.cgi?id=13555&action=view

Yes. The new patch adds just recomputes the rx buf size in one place, so it can be set when device is down.
Also, the computation will always yield same result now, the old code computed different values depending on whether it was initial or changed MTU.

But with your patch he is getting an OOPS (he described it as a 'segmentation fault' which is why you perhaps missed it) so you should try to figure out why so we can fix this bug.

:-)

Thanks.

#13 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

In reply to Stephen Hemminger
Quoted 5 lines · Stephen Hemminger
Quoted 2 lines · On Fri, 16 Nov 2007 10:21:25 -0600 "Jon Nelson" <[email protected]> wrote:

I get a segmentation fault.

Quoted 8 lines
Quoted 3 lines

Should this patch be in addition to the one from bug 9382?
http://bugzilla.kernel.org/attachment.cgi?id=13555&action=view

Yes. The new patch adds just recomputes the rx buf size in one place, so it can be set when device is down.
Also, the computation will always yield same result now, the old code computed different values depending on whether it was initial or changed MTU.

OK. This is what I did.
Using git I grabbed a copy of Linus' tree and using the latest files for via-velocity.[c,h], commit 99fee6d7e5748d96884667a4628118f7fc130ea0, I determined that if I backed out change 44c10138fd4bbc4b6d6bff0873c24902f2a9da65 (PCI:
Change all drivers to use pci_device->revision) I could get it to compile. This gets me a more recent driver.

Then I applied both of the patches you have provided me, and built and tried that.
No sigseg, no oops on initial MTU, no sigseg or oops on subsequent MTU changes.

Performance appears to be worse after MTU changes, though. I can get about 30MB/s with 7200 (the largest the board will support?) and about 5MB/s with 1500. If I *start out* with 1500 performance is 35-40MB/s. However, the immediate issue appears to be solved (oopses).

If there is a better tree to pull from, I'm more than willing to keep trying different drivers, at least for a while. I also have tg3 driver issues to sort out.

Signature · 1 line
Jon
#14 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

Jon Nelson wrote, On 11/17/2007 01:59 AM:
...

Quoted 11 lines

OK. This is what I did.
Using git I grabbed a copy of Linus' tree and using the latest files for via-velocity.[c,h], commit 99fee6d7e5748d96884667a4628118f7fc130ea0, I determined that if I backed out change 44c10138fd4bbc4b6d6bff0873c24902f2a9da65 (PCI:
Change all drivers to use pci_device->revision) I could get it to compile. This gets me a more recent driver.

Then I applied both of the patches you have provided me, and built and tried that.
No sigseg, no oops on initial MTU, no sigseg or oops on subsequent MTU changes.

Good news! But, if I got it right your method could be tricky. These current via-velocity files could depend on other files being current as well. So, it's safer to use the whole new kernel (eg. 2.6.24-rc3) or to stay with your older one. But, if you want to check the effect of these new two patches only without any additional 'features', your older kernel should be a better choice.

Regards,
Jarek P.

#15 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change.

In reply to Stephen Hemminger

Just a reminder: the following fix should (IMO) be considered for 2.6.24. The first patch does indeed fix the OOPS on MTU however subsequent MTU changes still OOPS. The below fixes that. I've been using both patches for over a week and they work just great. I noticed that the first fix made 2.6.24 but this one hadn't - I just want to try to make sure 2.6.24 doesn't make things /worse/ for via-velocity owners instead of better.

Quoted 40 lines · Stephen Hemminger

Does this fix the problem.

Note: reading the code the driver has other problems (besides crappy style). It does pci_map_single()/unmap in a way that doesn't account correctly for the padding that was added.

drivers/net/via-velocity.c 2007-11-15 20:32:14.000000000 -0800 renamed from drivers/net/via-velocity.c 2007-11-15 20:11:12.000000000 -0800 +3 -8
@@ -1242,6 +1242,9 @@ static int velocity_rx_refill(struct vel
 static int velocity_init_rd_ring(struct velocity_info *vptr)
 {
        int ret;
+       int mtu = vptr->dev->mtu;
+
+       vptr->rx_buf_sz = (mtu <= ETH_DATA_LEN) ? PKT_BUF_SZ : mtu + 32;
 
        vptr->rd_info = kcalloc(vptr->options.numrx,
                                sizeof(struct velocity_rd_info), GFP_KERNEL);

@@ -1898,8 +1901,6 @@ static int velocity_open(struct net_devi
        struct velocity_info *vptr = netdev_priv(dev);
        int ret;
 
-       vptr->rx_buf_sz = (dev->mtu <= 1504 ? PKT_BUF_SZ : dev->mtu + 32);
-
        ret = velocity_init_rings(vptr);
        if (ret < 0)
                goto out;

@@ -1978,12 +1979,6 @@ static int velocity_change_mtu(struct ne
                velocity_free_rd_ring(vptr);
 
                dev->mtu = new_mtu;
-               if (new_mtu > 8192)
-                       vptr->rx_buf_sz = 9 * 1024;
-               else if (new_mtu > 4096)
-                       vptr->rx_buf_sz = 8192;
-               else
-                       vptr->rx_buf_sz = 4 * 1024;
 
                ret = velocity_init_rd_ring(vptr);
                if (ret < 0)
Signature · 1 line
Jon
#16 lore raw

PATCH 1/1 via-velocity: don't oops on MTU change (resend)

The VIA veloicty driver needs the following to allow changing MTU when down. The buffer size needs to be computed when device is brought up, not when device is initialized. This also fixes a bug where the buffer size was computed differently on change_mtu versus initial setting.

Signed-off-by Stephen Hemminger <[email protected]>

---
This is a properly formatted version of previously submitted patch. Please apply for 2.6.24

drivers/net/via-velocity.c +3 -8
@@ -1242,6 +1242,9 @@ static int velocity_rx_refill(struct vel
 static int velocity_init_rd_ring(struct velocity_info *vptr)
 {
 	int ret;
+	int mtu = vptr->dev->mtu;
+
+	vptr->rx_buf_sz = (mtu <= ETH_DATA_LEN) ? PKT_BUF_SZ : mtu + 32;
 
 	vptr->rd_info = kcalloc(vptr->options.numrx,
 				sizeof(struct velocity_rd_info), GFP_KERNEL);

@@ -1898,8 +1901,6 @@ static int velocity_open(struct net_devi
 	struct velocity_info *vptr = netdev_priv(dev);
 	int ret;
 
-	vptr->rx_buf_sz = (dev->mtu <= 1504 ? PKT_BUF_SZ : dev->mtu + 32);
-
 	ret = velocity_init_rings(vptr);
 	if (ret < 0)
 		goto out;

@@ -1978,12 +1979,6 @@ static int velocity_change_mtu(struct ne
 		velocity_free_rd_ring(vptr);
 
 		dev->mtu = new_mtu;
-		if (new_mtu > 8192)
-			vptr->rx_buf_sz = 9 * 1024;
-		else if (new_mtu > 4096)
-			vptr->rx_buf_sz = 8192;
-		else
-			vptr->rx_buf_sz = 4 * 1024;
 
 		ret = velocity_init_rd_ring(vptr);
 		if (ret < 0)
Applied
#17 lore raw

Re: [PATCH] via-velocity: don't oops on MTU change (resend)

Quoted 10 lines · Stephen Hemminger

The VIA veloicty driver needs the following to allow changing MTU when down. The buffer size needs to be computed when device is brought up, not when device is initialized. This also fixes a bug where the buffer size was computed differently on change_mtu versus initial setting.

Signed-off-by Stephen Hemminger <[email protected]>

---
This is a properly formatted version of previously submitted patch. Please apply for 2.6.24

applied