From: Bart Trojanowski <[email protected]>
Date: Sat, 28 Feb 2009 13:05:41 -0500
> 2.6.29-rc* introduces a bug that causes a crash when a vlan is put into
> another vlan, so called vlan trunking or QinQ.
>
> I can reproduce it reliably with:
>
> $ modprobe 8021q
> $ vconfig add eth1 5
> $ ifconfig eth1.5 up
> $ vconfig add eth1.5 4
Stephen please fix this.
David Miller wrote:
> From: Bart Trojanowski <[email protected]>
> Date: Sat, 28 Feb 2009 13:05:41 -0500
>
>> 2.6.29-rc* introduces a bug that causes a crash when a vlan is put into
>> another vlan, so called vlan trunking or QinQ.
>>
>> I can reproduce it reliably with:
>>
>> $ modprobe 8021q
>> $ vconfig add eth1 5
>> $ ifconfig eth1.5 up
>> $ vconfig add eth1.5 4
>
> Stephen please fix this.
I'm maintaining vlan :) I haven't been able to look into this yet,
but I should be later today.
From: Patrick McHardy <[email protected]>
Date: Wed, 04 Mar 2009 10:57:47 +0100
> David Miller wrote:
> > From: Bart Trojanowski <[email protected]>
> > Date: Sat, 28 Feb 2009 13:05:41 -0500
> >
> >> 2.6.29-rc* introduces a bug that causes a crash when a vlan is put into
> >> another vlan, so called vlan trunking or QinQ.
> >>
> >> I can reproduce it reliably with:
> >>
> >> $ modprobe 8021q
> >> $ vconfig add eth1 5
> >> $ ifconfig eth1.5 up
> >> $ vconfig add eth1.5 4
> > Stephen please fix this.
>
> I'm maintaining vlan :) I haven't been able to look into this yet,
> but I should be later today.
Ok, I don't actually care who fixes it :-)
I asked Stephen because this appears like it might be netdev_ops
fallout.
David Miller wrote:
> From: Patrick McHardy <[email protected]>
> Date: Wed, 04 Mar 2009 10:57:47 +0100
>
>>>> I can reproduce it reliably with:
>>>>
>>>> $ modprobe 8021q
>>>> $ vconfig add eth1 5
>>>> $ ifconfig eth1.5 up
>>>> $ vconfig add eth1.5 4
>>> Stephen please fix this.
>> I'm maintaining vlan :) I haven't been able to look into this yet,
>> but I should be later today.
>
> Ok, I don't actually care who fixes it :-)
>
> I asked Stephen because this appears like it might be netdev_ops
> fallout.
Good point. I think I know whats happening. The VLAN devices are
registered with vlan_netdev_ops, the ->init function then potentially
replaces them based on whether the underlying device supports HW
acceleration. At this point the register_netdev() function already
has used the first netdev_ops structure to initialize the compat
pointers, meaning the new assignment is largely without effect and
causes incorrect ops to be used with HW acceleration
This probably causes the slab corruption since the HW accelerated
VLAN device doesn't increase the headroom, but the non-accelerated
functions try to add a hard header anyways.
This is a bit tricky to fix since we actually need some valid
ops before invoking ->init(). One way would be to move the compat
ops initialization to a seperate function and have VLAN use it to
switch its ops.
From: Patrick McHardy <[email protected]>
Date: Wed, 04 Mar 2009 12:45:33 +0100
> This is a bit tricky to fix since we actually need some valid
> ops before invoking ->init(). One way would be to move the compat
> ops initialization to a seperate function and have VLAN use it to
> switch its ops.
Mind if I push this into net-2.6?
vlan: Fix vlan-in-vlan crashes.
As analyzed by Patrick McHardy, vlan needs to reset it's
netdev_ops pointer in it's ->init() function but this
leaves the compat method pointers stale.
Add a netdev_resync_ops() and call it from the vlan code.
Signed-off-by: David S. Miller <[email protected]>
---
include/linux/netdevice.h | 1 +
net/8021q/vlan_dev.c | 1 +
net/core/dev.c | 54 +++++++++++++++++++++++++++-----------------
3 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ec54785..6593667 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1079,6 +1079,7 @@ extern void synchronize_net(void);
extern int register_netdevice_notifier(struct notifier_block *nb);
extern int unregister_netdevice_notifier(struct notifier_block *nb);
extern int init_dummy_netdev(struct net_device *dev);
+extern void netdev_resync_ops(struct net_device *dev);
extern int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
extern struct net_device *dev_get_by_index(struct net *net, int ifindex);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4a19acd..b6e1f9e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -639,6 +639,7 @@ static int vlan_dev_init(struct net_device *dev)
dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;
dev->netdev_ops = &vlan_netdev_ops;
}
+ netdev_resync_ops(dev);
if (is_vlan_dev(real_dev))
subclass = 1;
diff --git a/net/core/dev.c b/net/core/dev.c
index 9e4afe6..a06c6fa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4282,6 +4282,38 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
}
EXPORT_SYMBOL(netdev_fix_features);
+/* Some devices need to (re-)set their netdev_ops inside
+ * ->init() or similar. If that happens, we have to setup
+ * the compat pointers again.
+ */
+void netdev_resync_ops(struct net_device *dev)
+{
+#ifdef CONFIG_COMPAT_NET_DEV_OPS
+ const struct net_device_ops *ops = dev->netdev_ops;
+
+ dev->init = ops->ndo_init;
+ dev->uninit = ops->ndo_uninit;
+ dev->open = ops->ndo_open;
+ dev->change_rx_flags = ops->ndo_change_rx_flags;
+ dev->set_rx_mode = ops->ndo_set_rx_mode;
+ dev->set_multicast_list = ops->ndo_set_multicast_list;
+ dev->set_mac_address = ops->ndo_set_mac_address;
+ dev->validate_addr = ops->ndo_validate_addr;
+ dev->do_ioctl = ops->ndo_do_ioctl;
+ dev->set_config = ops->ndo_set_config;
+ dev->change_mtu = ops->ndo_change_mtu;
+ dev->tx_timeout = ops->ndo_tx_timeout;
+ dev->get_stats = ops->ndo_get_stats;
+ dev->vlan_rx_register = ops->ndo_vlan_rx_register;
+ dev->vlan_rx_add_vid = ops->ndo_vlan_rx_add_vid;
+ dev->vlan_rx_kill_vid = ops->ndo_vlan_rx_kill_vid;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ dev->poll_controller = ops->ndo_poll_controller;
+#endif
+#endif
+}
+EXPORT_SYMBOL(netdev_resync_ops);
+
/**
* register_netdevice - register a network device
* @dev: device to register
@@ -4326,27 +4358,7 @@ int register_netdevice(struct net_device *dev)
* This is temporary until all network devices are converted.
*/
if (dev->netdev_ops) {
- const struct net_device_ops *ops = dev->netdev_ops;
-
- dev->init = ops->ndo_init;
- dev->uninit = ops->ndo_uninit;
- dev->open = ops->ndo_open;
- dev->change_rx_flags = ops->ndo_change_rx_flags;
- dev->set_rx_mode = ops->ndo_set_rx_mode;
- dev->set_multicast_list = ops->ndo_set_multicast_list;
- dev->set_mac_address = ops->ndo_set_mac_address;
- dev->validate_addr = ops->ndo_validate_addr;
- dev->do_ioctl = ops->ndo_do_ioctl;
- dev->set_config = ops->ndo_set_config;
- dev->change_mtu = ops->ndo_change_mtu;
- dev->tx_timeout = ops->ndo_tx_timeout;
- dev->get_stats = ops->ndo_get_stats;
- dev->vlan_rx_register = ops->ndo_vlan_rx_register;
- dev->vlan_rx_add_vid = ops->ndo_vlan_rx_add_vid;
- dev->vlan_rx_kill_vid = ops->ndo_vlan_rx_kill_vid;
-#ifdef CONFIG_NET_POLL_CONTROLLER
- dev->poll_controller = ops->ndo_poll_controller;
-#endif
+ netdev_resync_ops(dev);
} else {
char drivername[64];
pr_info("%s (%s): not using net_device_ops yet\n",
--
1.6.2
David,
* David Miller <[email protected]> [090304 22:53]:
> vlan: Fix vlan-in-vlan crashes.
>
> As analyzed by Patrick McHardy, vlan needs to reset it's
> netdev_ops pointer in it's ->init() function but this
> leaves the compat method pointers stale.
>
> Add a netdev_resync_ops() and call it from the vlan code.
<snip>
> include/linux/netdevice.h | 1 +
> net/8021q/vlan_dev.c | 1 +
> net/core/dev.c | 54 +++++++++++++++++++++++++++-----------------
> 3 files changed, 35 insertions(+), 21 deletions(-)
I tried this patch onto v2.6.29-rc7-3-g559595a, but I still get a crash.
I assume that this worked for you, so I am not putting much faith in my
results at this late hour. I'll confirm tomorrow morning that it's not
something else.
Cheers,
-Bart
--
WebSig: http://www.jukie.net/~bart/sig/
David,
* Bart Trojanowski <[email protected]> [090304 23:54]:
> * David Miller <[email protected]> [090304 22:53]:
> > vlan: Fix vlan-in-vlan crashes.
> >
> > As analyzed by Patrick McHardy, vlan needs to reset it's
> > netdev_ops pointer in it's ->init() function but this
> > leaves the compat method pointers stale.
> >
> > Add a netdev_resync_ops() and call it from the vlan code.
> <snip>
> > include/linux/netdevice.h | 1 +
> > net/8021q/vlan_dev.c | 1 +
> > net/core/dev.c | 54 +++++++++++++++++++++++++++-----------------
> > 3 files changed, 35 insertions(+), 21 deletions(-)
>
> I tried this patch onto v2.6.29-rc7-3-g559595a, but I still get a crash.
> I assume that this worked for you, so I am not putting much faith in my
> results at this late hour. I'll confirm tomorrow morning that it's not
> something else.
... if you're interested, here is the Oops. And like I said, I'll
retest tomorrow.
-Bart
[ 231.748126] 802.1Q VLAN Support v1.8 Ben Greear <[email protected]>
[ 231.751563] All bugs added by David S. Miller <[email protected]>
[ 231.876164] PANIC: double fault, gdt at c364c000 [255 bytes]
[ 231.876271] double fault, tss at c364fae0
[ 231.876271] eip = f8163c65, esp = f3045000
[ 231.876271] eax = f3d78800, ebx = f3d78800, ecx = f8163c62, edx = f3e4ce40
[ 231.876271] esi = f3e4ce40, edi = c05228fc
[ 232.156110] BUG: unable to handle kernel paging request at a0ad0eb4
[ 232.159226] IP: [<c011fd49>] hrtick_start_fair+0x1f/0x17d
[ 232.160050] *pde = 00000000
[ 232.160050] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 232.160050] last sysfs file: /sys/class/net/lo/operstate
[ 232.160050] Modules linked in: 8021q virtio_balloon virtio_pci thermal_sys
[ 232.160050]
[ 232.160050] Pid: 0, comm: swapper Tainted: G S (2.6.29-rc7-bisect-00004-g39fc204 #1)
[ 232.160050] EIP: 0060:[<c011fd49>] EFLAGS: 00010092 CPU: 0
[ 232.160050] EIP is at hrtick_start_fair+0x1f/0x17d
[ 232.160050] EAX: c0599d80 EBX: c364e200 ECX: c3651dd4 EDX: f8163c85
[ 232.160050] ESI: f52f6198 EDI: f622b708 EBP: c0545ddc ESP: c0545dc4
[ 232.160050] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 232.160050] Process swapper (pid: 0, ti=c0544000 task=c04fd380 task.ti=c0544000)
[ 232.160050] Stack:
[ 232.160050] c3651d80 5f684afd 00000002 c3651dd4 f52f6198 c3651d80 c0545df0 c011ff82
[ 232.160050] f4796708 c03d6ad8 00000000 c0545e14 c011da63 0d94c928 00000036 00000000
[ 232.160050] c3651d80 c3651d80 c3648d80 00000522 c0545e20 c011daaa f4796708 c0545e34
[ 232.160050] Call Trace:
[ 232.160050] [<c011ff82>] ? dequeue_task_fair+0x51/0x56
[ 232.160050] [<c011da63>] ? dequeue_task+0xd5/0xe4
[ 232.160050] [<c011daaa>] ? deactivate_task+0x19/0x1f
[ 232.160050] [<c011e504>] ? pull_task+0x11/0x3d
[ 232.160050] [<c011e6fc>] ? load_balance_fair+0x134/0x1ce
[ 232.160050] [<c0124684>] ? rebalance_domains+0x219/0x421
[ 232.160050] [<c01248ba>] ? run_rebalance_domains+0x2e/0x9e
[ 232.160050] [<c012e0e7>] ? __do_softirq+0x8d/0x133
[ 232.160050] [<c012e1d5>] ? do_softirq+0x48/0x57
[ 232.160050] [<c012e2e6>] ? irq_exit+0x38/0x66
[ 232.160050] [<c011228b>] ? smp_apic_timer_interrupt+0x74/0x82
[ 232.160050] [<c0103cc0>] ? apic_timer_interrupt+0x28/0x30
[ 232.160050] [<c011824f>] ? native_safe_halt+0x5/0x7
[ 232.160050] [<c0108c92>] ? default_idle+0x30/0x58
[ 232.160050] [<c0102605>] ? cpu_idle+0x63/0x7e
[ 232.160050] [<c03bee3b>] ? rest_init+0x53/0x55
[ 232.160050] Code: 3e ff ff ff 83 c4 14 5b 5e 5f 5d c3 55 89 e5 57 89 d7 56 53 83 ec 0c 89 45 e8 8b 9a 80 00 00 00 b8 80 9d 59 c0 8b 52 04 8b 52 10 <03> 04 95 a0 1c 54 c0 39 45 e8 74 14 6a 00 68 7a 03 00 00 68 e1
[ 232.160050] EIP: [<c011fd49>] hrtick_start_fair+0x1f/0x17d SS:ESP 0068:c0545dc4
[ 232.160050] ---[ end trace b72565b053d710aa ]---
[ 232.160050] Kernel panic - not syncing: Fatal exception in interrupt
[ 232.160050] ------------[ cut here ]------------
[ 232.160050] WARNING: at kernel/smp.c:329 smp_call_function_many+0x37/0x1cf()
[ 232.160050] Hardware name:
[ 232.160050] Modules linked in: 8021q virtio_balloon virtio_pci thermal_sys
[ 232.160050] Pid: 0, comm: swapper Tainted: G S D 2.6.29-rc7-bisect-00004-g39fc204 #1
[ 232.160050] Call Trace:
[ 232.160050] [<c0129779>] warn_slowpath+0x71/0xa8
[ 232.160050] [<c0288f14>] ? _raw_spin_unlock+0x74/0x78
[ 232.160050] [<c03ce3aa>] ? _spin_unlock+0x1d/0x20
[ 232.160050] [<c0288f6b>] ? _raw_spin_lock+0x53/0xfa
[ 232.160050] [<c03ccadd>] ? mutex_unlock+0x8/0xa
[ 232.160050] [<c014b26d>] smp_call_function_many+0x37/0x1cf
[ 232.160050] [<c0108ec2>] ? stop_this_cpu+0x0/0x47
[ 232.160050] [<c014b421>] smp_call_function+0x1c/0x23
[ 232.160050] [<c0110e91>] native_smp_send_stop+0x1b/0x45
[ 232.160050] [<c03cb99e>] panic+0x48/0xe4
[ 232.160050] [<c0105ff4>] oops_end+0x9a/0xa9
[ 232.160050] [<c0106187>] die+0x54/0x5a
[ 232.160050] [<c01195d3>] do_page_fault+0x5f8/0x69d
[ 232.160050] [<c0119fe8>] ? __change_page_attr_set_clr+0x2a7/0x791
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0119fe8>] ? __change_page_attr_set_clr+0x2a7/0x791
[ 232.160050] [<c011a5b0>] ? kernel_map_pages+0xde/0xfe
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0118fdb>] ? do_page_fault+0x0/0x69d
[ 232.160050] [<c03ce82a>] error_code+0x72/0x78
[ 232.160050] [<f8163c85>] ? vlan_dev_neigh_setup+0x23/0x2a [8021q]
[ 232.160050] [<c011fd49>] ? hrtick_start_fair+0x1f/0x17d
[ 232.160050] [<c011ff82>] dequeue_task_fair+0x51/0x56
[ 232.160050] [<c011da63>] dequeue_task+0xd5/0xe4
[ 232.160050] [<c011daaa>] deactivate_task+0x19/0x1f
[ 232.160050] [<c011e504>] pull_task+0x11/0x3d
[ 232.160050] [<c011e6fc>] load_balance_fair+0x134/0x1ce
[ 232.160050] [<c0124684>] rebalance_domains+0x219/0x421
[ 232.160050] [<c01248ba>] run_rebalance_domains+0x2e/0x9e
[ 232.160050] [<c012e0e7>] __do_softirq+0x8d/0x133
[ 232.160050] [<c012e1d5>] do_softirq+0x48/0x57
[ 232.160050] [<c012e2e6>] irq_exit+0x38/0x66
[ 232.160050] [<c011228b>] smp_apic_timer_interrupt+0x74/0x82
[ 232.160050] [<c0103cc0>] apic_timer_interrupt+0x28/0x30
[ 232.160050] [<c011824f>] ? native_safe_halt+0x5/0x7
[ 232.160050] [<c0108c92>] default_idle+0x30/0x58
[ 232.160050] [<c0102605>] cpu_idle+0x63/0x7e
[ 232.160050] [<c03bee3b>] rest_init+0x53/0x55
[ 232.160050] ---[ end trace b72565b053d710ab ]---
[ 232.160050] ------------[ cut here ]------------
[ 232.160050] WARNING: at kernel/smp.c:226 smp_call_function_single+0x37/0xe8()
[ 232.160050] Hardware name:
[ 232.160050] Modules linked in: 8021q virtio_balloon virtio_pci thermal_sys
[ 232.160050] Pid: 0, comm: swapper Tainted: G S D W 2.6.29-rc7-bisect-00004-g39fc204 #1
[ 232.160050] Call Trace:
[ 232.160050] [<c0129779>] warn_slowpath+0x71/0xa8
[ 232.160050] [<c0288f14>] ? _raw_spin_unlock+0x74/0x78
[ 232.160050] [<c03ce300>] ? _read_unlock+0x15/0x20
[ 232.160050] [<c0288f6b>] ? _raw_spin_lock+0x53/0xfa
[ 232.160050] [<c014b185>] smp_call_function_single+0x37/0xe8
[ 232.160050] [<c0108ec2>] ? stop_this_cpu+0x0/0x47
[ 232.160050] [<c014b2ef>] smp_call_function_many+0xb9/0x1cf
[ 232.160050] [<c0108ec2>] ? stop_this_cpu+0x0/0x47
[ 232.160050] [<c014b421>] smp_call_function+0x1c/0x23
[ 232.160050] [<c0110e91>] native_smp_send_stop+0x1b/0x45
[ 232.160050] [<c03cb99e>] panic+0x48/0xe4
[ 232.160050] [<c0105ff4>] oops_end+0x9a/0xa9
[ 232.160050] [<c0106187>] die+0x54/0x5a
[ 232.160050] [<c01195d3>] do_page_fault+0x5f8/0x69d
[ 232.160050] [<c0119fe8>] ? __change_page_attr_set_clr+0x2a7/0x791
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0119fe8>] ? __change_page_attr_set_clr+0x2a7/0x791
[ 232.160050] [<c011a5b0>] ? kernel_map_pages+0xde/0xfe
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0119ca2>] ? lookup_address+0x68/0x88
[ 232.160050] [<c0118fdb>] ? do_page_fault+0x0/0x69d
[ 232.160050] [<c03ce82a>] error_code+0x72/0x78
[ 232.160050] [<f8163c85>] ? vlan_dev_neigh_setup+0x23/0x2a [8021q]
[ 232.160050] [<c011fd49>] ? hrtick_start_fair+0x1f/0x17d
[ 232.160050] [<c011ff82>] dequeue_task_fair+0x51/0x56
[ 232.160050] [<c011da63>] dequeue_task+0xd5/0xe4
[ 232.160050] [<c011daaa>] deactivate_task+0x19/0x1f
[ 232.160050] [<c011e504>] pull_task+0x11/0x3d
[ 232.160050] [<c011e6fc>] load_balance_fair+0x134/0x1ce
[ 232.160050] [<c0124684>] rebalance_domains+0x219/0x421
[ 232.160050] [<c01248ba>] run_rebalance_domains+0x2e/0x9e
[ 232.160050] [<c012e0e7>] __do_softirq+0x8d/0x133
[ 232.160050] [<c012e1d5>] do_softirq+0x48/0x57
[ 232.160050] [<c012e2e6>] irq_exit+0x38/0x66
[ 232.160050] [<c011228b>] smp_apic_timer_interrupt+0x74/0x82
[ 232.160050] [<c0103cc0>] apic_timer_interrupt+0x28/0x30
[ 232.160050] [<c011824f>] ? native_safe_halt+0x5/0x7
[ 232.160050] [<c0108c92>] default_idle+0x30/0x58
[ 232.160050] [<c0102605>] cpu_idle+0x63/0x7e
[ 232.160050] [<c03bee3b>] rest_init+0x53/0x55
[ 232.160050] ---[ end trace b72565b053d710ac ]---
From: Bart Trojanowski <[email protected]>
Date: Wed, 4 Mar 2009 23:54:42 -0500
> David,
>
> * David Miller <[email protected]> [090304 22:53]:
> > vlan: Fix vlan-in-vlan crashes.
> >
> > As analyzed by Patrick McHardy, vlan needs to reset it's
> > netdev_ops pointer in it's ->init() function but this
> > leaves the compat method pointers stale.
> >
> > Add a netdev_resync_ops() and call it from the vlan code.
> <snip>
> > include/linux/netdevice.h | 1 +
> > net/8021q/vlan_dev.c | 1 +
> > net/core/dev.c | 54 +++++++++++++++++++++++++++-----------------
> > 3 files changed, 35 insertions(+), 21 deletions(-)
>
> I tried this patch onto v2.6.29-rc7-3-g559595a, but I still get a crash.
> I assume that this worked for you, so I am not putting much faith in my
> results at this late hour. I'll confirm tomorrow morning that it's not
> something else.
I didn't test, so you the bug must be a different problem or
my patch is wrong :)
David Miller wrote:
> From: Patrick McHardy <[email protected]>
> Date: Wed, 04 Mar 2009 12:45:33 +0100
>
>> This is a bit tricky to fix since we actually need some valid
>> ops before invoking ->init(). One way would be to move the compat
>> ops initialization to a seperate function and have VLAN use it to
>> switch its ops.
>
> Mind if I push this into net-2.6?
>
> vlan: Fix vlan-in-vlan crashes.
>
> As analyzed by Patrick McHardy, vlan needs to reset it's
> netdev_ops pointer in it's ->init() function but this
> leaves the compat method pointers stale.
>
> Add a netdev_resync_ops() and call it from the vlan code.
This looks fine, thanks. Even if it doesn't fix this particular
report, I think its appropriate for net-2.6.
Bart Trojanowski wrote:
>>> As analyzed by Patrick McHardy, vlan needs to reset it's
>>> netdev_ops pointer in it's ->init() function but this
>>> leaves the compat method pointers stale.
>>>
>>> Add a netdev_resync_ops() and call it from the vlan code.
>> <snip>
>>> include/linux/netdevice.h | 1 +
>>> net/8021q/vlan_dev.c | 1 +
>>> net/core/dev.c | 54 +++++++++++++++++++++++++++-----------------
>>> 3 files changed, 35 insertions(+), 21 deletions(-)
>> I tried this patch onto v2.6.29-rc7-3-g559595a, but I still get a crash.
>> I assume that this worked for you, so I am not putting much faith in my
>> results at this late hour. I'll confirm tomorrow morning that it's not
>> something else.
>
> ... if you're interested, here is the Oops. And like I said, I'll
> retest tomorrow.
Thanks, I'll try to reproduce it here.
From: Patrick McHardy <[email protected]>
Date: Thu, 05 Mar 2009 06:51:25 +0100
> David Miller wrote:
> > From: Patrick McHardy <[email protected]>
> > Date: Wed, 04 Mar 2009 12:45:33 +0100
> >
> >> This is a bit tricky to fix since we actually need some valid
> >> ops before invoking ->init(). One way would be to move the compat
> >> ops initialization to a seperate function and have VLAN use it to
> >> switch its ops.
> > Mind if I push this into net-2.6?
> > vlan: Fix vlan-in-vlan crashes.
> > As analyzed by Patrick McHardy, vlan needs to reset it's
> > netdev_ops pointer in it's ->init() function but this
> > leaves the compat method pointers stale.
> > Add a netdev_resync_ops() and call it from the vlan code.
>
> This looks fine, thanks. Even if it doesn't fix this particular
> report, I think its appropriate for net-2.6.
Thanks for looking at it, but I don't want to push it out until we
fully resolve this bug.
I think this will fix it.
diff --git a/net/core/dev.c b/net/core/dev.c
index 9e4afe6..2dd484e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4339,6 +4339,7 @@ int register_netdevice(struct net_device *dev)
dev->do_ioctl = ops->ndo_do_ioctl;
dev->set_config = ops->ndo_set_config;
dev->change_mtu = ops->ndo_change_mtu;
+ dev->neigh_setup = ops->ndo_neigh_setup;
dev->tx_timeout = ops->ndo_tx_timeout;
dev->get_stats = ops->ndo_get_stats;
dev->vlan_rx_register = ops->ndo_vlan_rx_register;
David Miller wrote:
> I think this will fix it.
Good catch, that looks like a likely cause. Will test in a minute ...
From: Patrick McHardy <[email protected]>
Date: Thu, 05 Mar 2009 08:05:10 +0100
> David Miller wrote:
> > I think this will fix it.
>
> Good catch, that looks like a likely cause. Will test in a minute ...
We probably need both fixes to cover everything.
David Miller wrote:
> From: Patrick McHardy <[email protected]>
> Date: Thu, 05 Mar 2009 08:05:10 +0100
>
>> David Miller wrote:
>>> I think this will fix it.
>> Good catch, that looks like a likely cause. Will test in a minute ...
>
> We probably need both fixes to cover everything.
>
Yes, just the second one still crashes. I'm about to retry using both.
From: Patrick McHardy <[email protected]>
Date: Thu, 05 Mar 2009 08:12:50 +0100
> David Miller wrote:
> > From: Patrick McHardy <[email protected]>
> > Date: Thu, 05 Mar 2009 08:05:10 +0100
> >
> >> David Miller wrote:
> >>> I think this will fix it.
> >> Good catch, that looks like a likely cause. Will test in a minute ...
> > We probably need both fixes to cover everything.
> >
>
> Yes, just the second one still crashes. I'm about to retry using both.
Here is the updated version just for the record:
vlan: Fix vlan-in-vlan crashes.
As analyzed by Patrick McHardy, vlan needs to reset it's
netdev_ops pointer in it's ->init() function but this
leaves the compat method pointers stale.
Add a netdev_resync_ops() and call it from the vlan code.
Any other driver which changes ->netdev_ops after register_netdevice()
will need to call this new function after doing so too.
Signed-off-by: David S. Miller <[email protected]>
---
include/linux/netdevice.h | 1 +
net/8021q/vlan_dev.c | 1 +
net/core/dev.c | 56 +++++++++++++++++++++++++++-----------------
3 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ec54785..6593667 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1079,6 +1079,7 @@ extern void synchronize_net(void);
extern int register_netdevice_notifier(struct notifier_block *nb);
extern int unregister_netdevice_notifier(struct notifier_block *nb);
extern int init_dummy_netdev(struct net_device *dev);
+extern void netdev_resync_ops(struct net_device *dev);
extern int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
extern struct net_device *dev_get_by_index(struct net *net, int ifindex);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4a19acd..b6e1f9e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -639,6 +639,7 @@ static int vlan_dev_init(struct net_device *dev)
dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;
dev->netdev_ops = &vlan_netdev_ops;
}
+ netdev_resync_ops(dev);
if (is_vlan_dev(real_dev))
subclass = 1;
diff --git a/net/core/dev.c b/net/core/dev.c
index 2dd484e..f112970 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4282,6 +4282,39 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
}
EXPORT_SYMBOL(netdev_fix_features);
+/* Some devices need to (re-)set their netdev_ops inside
+ * ->init() or similar. If that happens, we have to setup
+ * the compat pointers again.
+ */
+void netdev_resync_ops(struct net_device *dev)
+{
+#ifdef CONFIG_COMPAT_NET_DEV_OPS
+ const struct net_device_ops *ops = dev->netdev_ops;
+
+ dev->init = ops->ndo_init;
+ dev->uninit = ops->ndo_uninit;
+ dev->open = ops->ndo_open;
+ dev->change_rx_flags = ops->ndo_change_rx_flags;
+ dev->set_rx_mode = ops->ndo_set_rx_mode;
+ dev->set_multicast_list = ops->ndo_set_multicast_list;
+ dev->set_mac_address = ops->ndo_set_mac_address;
+ dev->validate_addr = ops->ndo_validate_addr;
+ dev->do_ioctl = ops->ndo_do_ioctl;
+ dev->set_config = ops->ndo_set_config;
+ dev->change_mtu = ops->ndo_change_mtu;
+ dev->neigh_setup = ops->ndo_neigh_setup;
+ dev->tx_timeout = ops->ndo_tx_timeout;
+ dev->get_stats = ops->ndo_get_stats;
+ dev->vlan_rx_register = ops->ndo_vlan_rx_register;
+ dev->vlan_rx_add_vid = ops->ndo_vlan_rx_add_vid;
+ dev->vlan_rx_kill_vid = ops->ndo_vlan_rx_kill_vid;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ dev->poll_controller = ops->ndo_poll_controller;
+#endif
+#endif
+}
+EXPORT_SYMBOL(netdev_resync_ops);
+
/**
* register_netdevice - register a network device
* @dev: device to register
@@ -4326,28 +4359,7 @@ int register_netdevice(struct net_device *dev)
* This is temporary until all network devices are converted.
*/
if (dev->netdev_ops) {
- const struct net_device_ops *ops = dev->netdev_ops;
-
- dev->init = ops->ndo_init;
- dev->uninit = ops->ndo_uninit;
- dev->open = ops->ndo_open;
- dev->change_rx_flags = ops->ndo_change_rx_flags;
- dev->set_rx_mode = ops->ndo_set_rx_mode;
- dev->set_multicast_list = ops->ndo_set_multicast_list;
- dev->set_mac_address = ops->ndo_set_mac_address;
- dev->validate_addr = ops->ndo_validate_addr;
- dev->do_ioctl = ops->ndo_do_ioctl;
- dev->set_config = ops->ndo_set_config;
- dev->change_mtu = ops->ndo_change_mtu;
- dev->neigh_setup = ops->ndo_neigh_setup;
- dev->tx_timeout = ops->ndo_tx_timeout;
- dev->get_stats = ops->ndo_get_stats;
- dev->vlan_rx_register = ops->ndo_vlan_rx_register;
- dev->vlan_rx_add_vid = ops->ndo_vlan_rx_add_vid;
- dev->vlan_rx_kill_vid = ops->ndo_vlan_rx_kill_vid;
-#ifdef CONFIG_NET_POLL_CONTROLLER
- dev->poll_controller = ops->ndo_poll_controller;
-#endif
+ netdev_resync_ops(dev);
} else {
char drivername[64];
pr_info("%s (%s): not using net_device_ops yet\n",
--
1.6.2
David Miller wrote:
> From: Patrick McHardy <[email protected]>
> Date: Thu, 05 Mar 2009 08:12:50 +0100
>
>>> We probably need both fixes to cover everything.
>>>
>> Yes, just the second one still crashes. I'm about to retry using both.
>
> Here is the updated version just for the record:
>
> vlan: Fix vlan-in-vlan crashes.
This still crashes. I'll have another look at the code.
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4a19acd..1b34135 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -553,7 +553,7 @@ static int vlan_dev_neigh_setup(struct net_device *dev, struct neigh_parms *pa)
int err = 0;
if (netif_device_present(real_dev) && ops->ndo_neigh_setup)
- err = ops->ndo_neigh_setup(dev, pa);
+ err = ops->ndo_neigh_setup(real_dev, pa);
return err;
}
From: Patrick McHardy <[email protected]>
Date: Thu, 05 Mar 2009 08:31:31 +0100
> Patrick McHardy wrote:
> > David Miller wrote:
> >> From: Patrick McHardy <[email protected]>
> >> Date: Thu, 05 Mar 2009 08:12:50 +0100
> >>
> >>>> We probably need both fixes to cover everything.
> >>>>
> >>> Yes, just the second one still crashes. I'm about to retry using both.
> >>
> >> Here is the updated version just for the record:
> >>
> >> vlan: Fix vlan-in-vlan crashes.
> > This still crashes. I'll have another look at the code.
>
> This one combined with your patch fixes the crash. The code was calling
> vlan_dev_neigh_setup recursively.
>
> Signed-off-by: Patrick McHardy <[email protected]>
>
> (or Tested-by: in case you want to roll it into your patch).
Great work, thanks Patrick!
I'll get this all integrated and push it out to Linus.
Hi Dave, Patrick,
sorry I could not follow the complete discussion of the fixes done for this problem
but does
if (netif_device_present(real_dev) && ops->ndo_neigh_setup)
- err = ops->ndo_neigh_setup(dev, pa);
+ err = ops->ndo_neigh_setup(real_dev, pa);
not change the idea of the neigh_setup? Remind we want the neigh_setup of the
real device as the neigh setup function for the vlan device.
Frank
Patrick McHardy schrieb:
> Patrick McHardy wrote:
>> David Miller wrote:
>>> From: Patrick McHardy <[email protected]>
>>> Date: Thu, 05 Mar 2009 08:12:50 +0100
>>>
>>>>> We probably need both fixes to cover everything.
>>>>>
>>>> Yes, just the second one still crashes. I'm about to retry using both.
>>>
>>> Here is the updated version just for the record:
>>>
>>> vlan: Fix vlan-in-vlan crashes.
>>
>> This still crashes. I'll have another look at the code.
>
> This one combined with your patch fixes the crash. The code was calling
> vlan_dev_neigh_setup recursively.
>
> Signed-off-by: Patrick McHardy <[email protected]>
>
> (or Tested-by: in case you want to roll it into your patch).
>
Frank Blaschka wrote:
> Hi Dave, Patrick,
>
> sorry I could not follow the complete discussion of the fixes done for this problem
> but does
>
> if (netif_device_present(real_dev) && ops->ndo_neigh_setup)
> - err = ops->ndo_neigh_setup(dev, pa);
> + err = ops->ndo_neigh_setup(real_dev, pa);
>
> not change the idea of the neigh_setup? Remind we want the neigh_setup of the
> real device as the neigh setup function for the vlan device.
>
An we still use it. The only difference is that we pass it the
correct device reference, which not only fixes the recursion,
but is also expected by the callbacks. Look at bonding or simply
vlan itself.
The setup itself is still done using the neigh_params passed to
VLAN, which appears to be what was originally intended.
From: Patrick McHardy <[email protected]>
Date: Thu, 05 Mar 2009 09:27:12 +0100
> Frank Blaschka wrote:
> > Hi Dave, Patrick,
> >
> > sorry I could not follow the complete discussion of the fixes done for this problem
> > but does
> >
> > if (netif_device_present(real_dev) && ops->ndo_neigh_setup)
> > - err = ops->ndo_neigh_setup(dev, pa);
> > + err = ops->ndo_neigh_setup(real_dev, pa);
> >
> > not change the idea of the neigh_setup? Remind we want the neigh_setup of the
> > real device as the neigh setup function for the vlan device.
> >
>
> An we still use it. The only difference is that we pass it the
> correct device reference, which not only fixes the recursion,
> but is also expected by the callbacks. Look at bonding or simply
> vlan itself.
>
> The setup itself is still done using the neigh_params passed to
> VLAN, which appears to be what was originally intended.
Then bond_neigh_setup() has the same bug, doesn't it?
From: David Miller <[email protected]>
Date: Thu, 05 Mar 2009 00:56:46 -0800 (PST)
> Then bond_neigh_setup() has the same bug, doesn't it?
Looking at the bond_main.c changes in:
commit 008298231abbeb91bc7be9e8b078607b816d1a4a
Author: Stephen Hemminger <[email protected]>
Date: Thu Nov 20 20:14:53 2008 -0800
netdev: add more functions to netdevice ops
shows that it always behaved that way.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9fb3883..383ce48 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4113,7 +4113,7 @@ static int bond_neigh_setup(struct net_device *dev, struct neigh_parms *parms)
const struct net_device_ops *slave_ops
= slave->dev->netdev_ops;
if (slave_ops->ndo_neigh_setup)
- return slave_ops->ndo_neigh_setup(dev, parms);
+ return slave_ops->ndo_neigh_setup(slave, parms);
}
return 0;
}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9fb3883..e0578fe 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4113,7 +4113,7 @@ static int bond_neigh_setup(struct net_device *dev, struct neigh_parms *parms)
const struct net_device_ops *slave_ops
= slave->dev->netdev_ops;
if (slave_ops->ndo_neigh_setup)
- return slave_ops->ndo_neigh_setup(dev, parms);
+ return slave_ops->ndo_neigh_setup(slave->dev, parms);
}
return 0;
}
From: Patrick McHardy <[email protected]>
Date: Thu, 05 Mar 2009 10:09:18 +0100
> Patrick McHardy wrote:
> > Yes, but that patch introduced the requirement to pass the correct
> > device down since now the handlers need it to get to the ops of the
> > underlying device. Previously they all relied on the handlers not
> > using their private data.
Aha, that's right.
> > Signed-off-by: Patrick McHardy <[email protected]>
> >
>
> Oops, the last patch was broken.
Applied, thanks!
On Wed, 2009-03-04 at 19:53 -0800, David Miller wrote:
Hi David,
> +#ifdef CONFIG_COMPAT_NET_DEV_OPS
> + const struct net_device_ops *ops = dev->netdev_ops;
> +
> + dev->init = ops->ndo_init;
> + dev->uninit = ops->ndo_uninit;
> + dev->open = ops->ndo_open;
> + dev->change_rx_flags = ops->ndo_change_rx_flags;
> + dev->set_rx_mode = ops->ndo_set_rx_mode;
> + dev->set_multicast_list = ops->ndo_set_multicast_list;
> + dev->set_mac_address = ops->ndo_set_mac_address;
> + dev->validate_addr = ops->ndo_validate_addr;
> + dev->do_ioctl = ops->ndo_do_ioctl;
> + dev->set_config = ops->ndo_set_config;
> + dev->change_mtu = ops->ndo_change_mtu;
> + dev->tx_timeout = ops->ndo_tx_timeout;
> + dev->get_stats = ops->ndo_get_stats;
> + dev->vlan_rx_register = ops->ndo_vlan_rx_register;
> + dev->vlan_rx_add_vid = ops->ndo_vlan_rx_add_vid;
> + dev->vlan_rx_kill_vid = ops->ndo_vlan_rx_kill_vid;
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + dev->poll_controller = ops->ndo_poll_controller;
> +#endif
> +#endif
> +}
> +EXPORT_SYMBOL(netdev_resync_ops);
Any reason dev->stop is not in this list ?
Regards,
--
Maxime
From: Maxime Bizon <[email protected]>
Date: Thu, 05 Mar 2009 13:30:26 +0100
> Any reason dev->stop is not in this list ?
dev->stop is only called from net/core/dev.c and it is
netdev_ops aware :-)