Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On 03/19/2013 01:59 PM, Eric Dumazet wrote: On Tue, 2013-03-19 at 13:58 +0100, Daniel Borkmann wrote: Yes, will post them in a couple of minutes. Please target net tree for the first patch (adding thoff into struct flow_keys), so that Jason or me can fix DODGY providers. Sorry, I received this too late. The patch set is already out, but we can put a note into the ``[PATCH net-next 1/4] flow_keys: include thoff into flow_keys for later'' thread to let Dave know. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On Tue, 2013-03-19 at 13:58 +0100, Daniel Borkmann wrote: > Yes, will post them in a couple of minutes. > Please target net tree for the first patch (adding thoff into struct flow_keys), so that Jason or me can fix DODGY providers. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On 03/19/2013 01:13 PM, Eric Dumazet wrote: On Tue, 2013-03-19 at 17:26 +0800, Jason Wang wrote: On 03/18/2013 12:13 AM, David Miller wrote: From: Eric Dumazet Date: Fri, 15 Mar 2013 19:10:51 -0700 Any way we can avoid adding this to fast path, for people not using macvtap and ixgbe ? Likewise I'd rather see macvtap be responsible for fixing this up by setting the transport header properly, and therfore sending well formed packets to the rest of the stack. Ok, haven't checked all other possibility but looks like packet needs to be fixed also. Daniel, could you post your patches if ready ? Yes, will post them in a couple of minutes. Jason, I believe you could reuse existing flow dissector once Daniel patches are in. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On Tue, 2013-03-19 at 17:26 +0800, Jason Wang wrote: > On 03/18/2013 12:13 AM, David Miller wrote: > > From: Eric Dumazet > > Date: Fri, 15 Mar 2013 19:10:51 -0700 > > > >> Any way we can avoid adding this to fast path, for people not using > >> macvtap and ixgbe ? > > Likewise I'd rather see macvtap be responsible for fixing this up by > > setting the transport header properly, and therfore sending well > > formed packets to the rest of the stack. > > Ok, haven't checked all other possibility but looks like packet needs to > be fixed also. Daniel, could you post your patches if ready ? Jason, I believe you could reuse existing flow dissector once Daniel patches are in. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On 03/18/2013 12:13 AM, David Miller wrote: > From: Eric Dumazet > Date: Fri, 15 Mar 2013 19:10:51 -0700 > >> Any way we can avoid adding this to fast path, for people not using >> macvtap and ixgbe ? > Likewise I'd rather see macvtap be responsible for fixing this up by > setting the transport header properly, and therfore sending well > formed packets to the rest of the stack. Ok, haven't checked all other possibility but looks like packet needs to be fixed also. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On 03/18/2013 12:13 AM, David Miller wrote: From: Eric Dumazet eric.duma...@gmail.com Date: Fri, 15 Mar 2013 19:10:51 -0700 Any way we can avoid adding this to fast path, for people not using macvtap and ixgbe ? Likewise I'd rather see macvtap be responsible for fixing this up by setting the transport header properly, and therfore sending well formed packets to the rest of the stack. Ok, haven't checked all other possibility but looks like packet needs to be fixed also. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On Tue, 2013-03-19 at 17:26 +0800, Jason Wang wrote: On 03/18/2013 12:13 AM, David Miller wrote: From: Eric Dumazet eric.duma...@gmail.com Date: Fri, 15 Mar 2013 19:10:51 -0700 Any way we can avoid adding this to fast path, for people not using macvtap and ixgbe ? Likewise I'd rather see macvtap be responsible for fixing this up by setting the transport header properly, and therfore sending well formed packets to the rest of the stack. Ok, haven't checked all other possibility but looks like packet needs to be fixed also. Daniel, could you post your patches if ready ? Jason, I believe you could reuse existing flow dissector once Daniel patches are in. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On 03/19/2013 01:13 PM, Eric Dumazet wrote: On Tue, 2013-03-19 at 17:26 +0800, Jason Wang wrote: On 03/18/2013 12:13 AM, David Miller wrote: From: Eric Dumazet eric.duma...@gmail.com Date: Fri, 15 Mar 2013 19:10:51 -0700 Any way we can avoid adding this to fast path, for people not using macvtap and ixgbe ? Likewise I'd rather see macvtap be responsible for fixing this up by setting the transport header properly, and therfore sending well formed packets to the rest of the stack. Ok, haven't checked all other possibility but looks like packet needs to be fixed also. Daniel, could you post your patches if ready ? Yes, will post them in a couple of minutes. Jason, I believe you could reuse existing flow dissector once Daniel patches are in. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On Tue, 2013-03-19 at 13:58 +0100, Daniel Borkmann wrote: Yes, will post them in a couple of minutes. Please target net tree for the first patch (adding thoff into struct flow_keys), so that Jason or me can fix DODGY providers. Thanks -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On 03/19/2013 01:59 PM, Eric Dumazet wrote: On Tue, 2013-03-19 at 13:58 +0100, Daniel Borkmann wrote: Yes, will post them in a couple of minutes. Please target net tree for the first patch (adding thoff into struct flow_keys), so that Jason or me can fix DODGY providers. Sorry, I received this too late. The patch set is already out, but we can put a note into the ``[PATCH net-next 1/4] flow_keys: include thoff into flow_keys for later'' thread to let Dave know. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
From: Eric Dumazet Date: Fri, 15 Mar 2013 19:10:51 -0700 > Any way we can avoid adding this to fast path, for people not using > macvtap and ixgbe ? Likewise I'd rather see macvtap be responsible for fixing this up by setting the transport header properly, and therfore sending well formed packets to the rest of the stack. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
From: Eric Dumazet eric.duma...@gmail.com Date: Fri, 15 Mar 2013 19:10:51 -0700 Any way we can avoid adding this to fast path, for people not using macvtap and ixgbe ? Likewise I'd rather see macvtap be responsible for fixing this up by setting the transport header properly, and therfore sending well formed packets to the rest of the stack. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On Fri, 2013-03-15 at 15:41 +0800, Jason Wang wrote: > Some drivers depends on transport_header to do packet transmission, but it was > unset in some cases (one example is macvtap driver which build skbs from > userspace and generate CHECKSUM_NONE packets). The driver may crash in those > cases since the transport_header was not valid. The problem becomes more > obvious > since commit fda55eca5a33f33ffcd4192c6b2d75179714a52c (net: introduce > skb_transport_header_was_set()) since it initializes transport_header to ~0U. > > So before passing the skb to driver, this patch reset the transport_header if > it > was not set to avoid such crash such as: > > hp-z800-04.qe.lab.eng.nay.redhat.com login: BUG: unable to handle kernel > paging > request at 8805166f760c > IP: [] ixgbe_xmit_frame_ring+0x220/0x5e0 [ixgbe] > PGD 1ece067 PUD 0 > Oops: [#1] SMP > Modules linked in: vhost_net tun nfsv3 nfs_acl nfsv4 auth_rpcgss nfs fscache > lockd autofs4 sunrpc openvswitch ipv6 iTCO_wdt iTCO_vendor_support hp_wmi > sparse_keymap rfkill acpi_cpufreq freq_table mperf coretemp kvm_intel kvm > crc32c_intel ghash_clmulni_intel microcode serio_raw pcspkr sg lpc_ich > mfd_core > tg3 snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq > snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc i7core_edac > edac_core ixgbe dca ptp pps_core mdio ext4(F) mbcache(F) jbd2(F) sd_mod(F) > crc_t10dif(F) sr_mod(F) cdrom(F) firewire_ohci(F) firewire_core(F) > crc_itu_t(F) > aesni_intel(F) ablk_helper(F) cryptd(F) lrw(F) aes_x86_64(F) xts(F) > gf128mul(F) > floppy(F) mptsas(F) mptscsih(F) mptbase(F) scsi_transport_sas(F) ahci(F) > libahci(F) nouveau(F) ttm(F) drm_kms_helper(F) drm(F) i2c_algo_bit(F) > i2c_core(F) mxm_wmi(F) video(F) wmi(F) dm_mirror(F) dm_region_hash(F) > dm_log(F) > dm_mod(F) [last unloaded: tun] > CPU 6 > Pid: 17337, comm: vhost-17317 Tainted: GF3.9.0-rc1+ #7 > Hewlett-Packard HP Z800 Workstation/0AECh > RIP: 0010:[] [] > ixgbe_xmit_frame_ring+0x220/0x5e0 [ixgbe] > RSP: 0018:880222cddb18 EFLAGS: 00010286 > RAX: RBX: 880416b4b000 RCX: 8805166f75ff > RDX: 0008 RSI: 8804166f760e RDI: 0007 > RBP: 880222cddb68 R08: 0008 R09: > R10: R11: R12: c90009dce120 > R13: 880416b4b300 R14: R15: 8804118f0800 > FS: () GS:88042fc4() knlGS: > CS: 0010 DS: ES: CR0: 8005003b > CR2: 8805166f760c CR3: 00041e98c000 CR4: 27e0 > DR0: DR1: DR2: > DR3: DR6: 0ff0 DR7: 0400 > Process vhost-17317 (pid: 17337, threadinfo 880222cdc000, task > 8802211d4040) > Stack: > 0180 880222cddbb7 0180 > 880222cddb48 88040d5dd1c0 8804118f 0036 > 8804118f 8804165d7a9c 880222cddb88 a035a9d3 > Call Trace: > [] ixgbe_xmit_frame+0x43/0x90 [ixgbe] > [] dev_hard_start_xmit+0x12a/0x570 > [] sch_direct_xmit+0xfa/0x1d0 > [] dev_queue_xmit+0x198/0x4c0 > [] macvlan_start_xmit+0x6a/0x170 > [] macvtap_get_user+0x404/0x4e0 > [] macvtap_sendmsg+0x2b/0x30 > [] handle_tx+0x34a/0x680 [vhost_net] > [] handle_tx_kick+0x15/0x20 [vhost_net] > [] vhost_worker+0x10c/0x1c0 [vhost_net] > [] ? vhost_attach_cgroups_work+0x30/0x30 [vhost_net] > [] ? vhost_attach_cgroups_work+0x30/0x30 [vhost_net] > [] kthread+0xce/0xe0 > [] ? kthread_freezable_should_stop+0x70/0x70 > [] ret_from_fork+0x7c/0xb0 > [] ? kthread_freezable_should_stop+0x70/0x70 > Code: 34 31 0f 84 d3 01 00 00 66 83 fa 08 0f 85 b9 00 00 00 80 7e 09 06 0f 85 > af > 00 00 00 8b 80 cc 00 00 00 48 01 c1 0f 84 a0 00 00 00 <0f> b6 41 0d a8 01 0f > 85 > 94 00 00 00 a8 02 75 0a 41 3a 7d 5c 0f > RIP [] ixgbe_xmit_frame_ring+0x220/0x5e0 [ixgbe] > RSP > CR2: 8805166f760c > > Cc: Eric Dumazet > Signed-off-by: Jason Wang > --- > net/core/dev.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 480114d..db315a1 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2525,6 +2525,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct > net_device *dev, > } > } > > + if (!skb_transport_header_was_set(skb)) > + skb_reset_transport_header(skb); > + > if (!list_empty(_all)) > dev_queue_xmit_nit(skb, dev); > Hmm... This really looks strange. Any way we can avoid adding this to fast path, for people not using macvtap and ixgbe ? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
Re: [PATCH net-next 2/2] net: reset transport header if it was not set before transmission
On Fri, 2013-03-15 at 15:41 +0800, Jason Wang wrote: Some drivers depends on transport_header to do packet transmission, but it was unset in some cases (one example is macvtap driver which build skbs from userspace and generate CHECKSUM_NONE packets). The driver may crash in those cases since the transport_header was not valid. The problem becomes more obvious since commit fda55eca5a33f33ffcd4192c6b2d75179714a52c (net: introduce skb_transport_header_was_set()) since it initializes transport_header to ~0U. So before passing the skb to driver, this patch reset the transport_header if it was not set to avoid such crash such as: hp-z800-04.qe.lab.eng.nay.redhat.com login: BUG: unable to handle kernel paging request at 8805166f760c IP: [a035a5d0] ixgbe_xmit_frame_ring+0x220/0x5e0 [ixgbe] PGD 1ece067 PUD 0 Oops: [#1] SMP Modules linked in: vhost_net tun nfsv3 nfs_acl nfsv4 auth_rpcgss nfs fscache lockd autofs4 sunrpc openvswitch ipv6 iTCO_wdt iTCO_vendor_support hp_wmi sparse_keymap rfkill acpi_cpufreq freq_table mperf coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel microcode serio_raw pcspkr sg lpc_ich mfd_core tg3 snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc i7core_edac edac_core ixgbe dca ptp pps_core mdio ext4(F) mbcache(F) jbd2(F) sd_mod(F) crc_t10dif(F) sr_mod(F) cdrom(F) firewire_ohci(F) firewire_core(F) crc_itu_t(F) aesni_intel(F) ablk_helper(F) cryptd(F) lrw(F) aes_x86_64(F) xts(F) gf128mul(F) floppy(F) mptsas(F) mptscsih(F) mptbase(F) scsi_transport_sas(F) ahci(F) libahci(F) nouveau(F) ttm(F) drm_kms_helper(F) drm(F) i2c_algo_bit(F) i2c_core(F) mxm_wmi(F) video(F) wmi(F) dm_mirror(F) dm_region_hash(F) dm_log(F) dm_mod(F) [last unloaded: tun] CPU 6 Pid: 17337, comm: vhost-17317 Tainted: GF3.9.0-rc1+ #7 Hewlett-Packard HP Z800 Workstation/0AECh RIP: 0010:[a035a5d0] [a035a5d0] ixgbe_xmit_frame_ring+0x220/0x5e0 [ixgbe] RSP: 0018:880222cddb18 EFLAGS: 00010286 RAX: RBX: 880416b4b000 RCX: 8805166f75ff RDX: 0008 RSI: 8804166f760e RDI: 0007 RBP: 880222cddb68 R08: 0008 R09: R10: R11: R12: c90009dce120 R13: 880416b4b300 R14: R15: 8804118f0800 FS: () GS:88042fc4() knlGS: CS: 0010 DS: ES: CR0: 8005003b CR2: 8805166f760c CR3: 00041e98c000 CR4: 27e0 DR0: DR1: DR2: DR3: DR6: 0ff0 DR7: 0400 Process vhost-17317 (pid: 17337, threadinfo 880222cdc000, task 8802211d4040) Stack: 0180 880222cddbb7 0180 880222cddb48 88040d5dd1c0 8804118f 0036 8804118f 8804165d7a9c 880222cddb88 a035a9d3 Call Trace: [a035a9d3] ixgbe_xmit_frame+0x43/0x90 [ixgbe] [8149d54a] dev_hard_start_xmit+0x12a/0x570 [814bd8da] sch_direct_xmit+0xfa/0x1d0 [8149db28] dev_queue_xmit+0x198/0x4c0 [813d23fa] macvlan_start_xmit+0x6a/0x170 [813d3974] macvtap_get_user+0x404/0x4e0 [813d3a7b] macvtap_sendmsg+0x2b/0x30 [a06d9efa] handle_tx+0x34a/0x680 [vhost_net] [a06da265] handle_tx_kick+0x15/0x20 [vhost_net] [a06d7dfc] vhost_worker+0x10c/0x1c0 [vhost_net] [a06d7cf0] ? vhost_attach_cgroups_work+0x30/0x30 [vhost_net] [a06d7cf0] ? vhost_attach_cgroups_work+0x30/0x30 [vhost_net] [8107b77e] kthread+0xce/0xe0 [8107b6b0] ? kthread_freezable_should_stop+0x70/0x70 [815749ec] ret_from_fork+0x7c/0xb0 [8107b6b0] ? kthread_freezable_should_stop+0x70/0x70 Code: 34 31 0f 84 d3 01 00 00 66 83 fa 08 0f 85 b9 00 00 00 80 7e 09 06 0f 85 af 00 00 00 8b 80 cc 00 00 00 48 01 c1 0f 84 a0 00 00 00 0f b6 41 0d a8 01 0f 85 94 00 00 00 a8 02 75 0a 41 3a 7d 5c 0f RIP [a035a5d0] ixgbe_xmit_frame_ring+0x220/0x5e0 [ixgbe] RSP 880222cddb18 CR2: 8805166f760c Cc: Eric Dumazet eduma...@google.com Signed-off-by: Jason Wang jasow...@redhat.com --- net/core/dev.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 480114d..db315a1 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2525,6 +2525,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, } } + if (!skb_transport_header_was_set(skb)) + skb_reset_transport_header(skb); + if (!list_empty(ptype_all)) dev_queue_xmit_nit(skb, dev); Hmm... This really looks strange. Any way we can avoid adding this to fast path, for
