Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Mi, 2015-01-28 at 15:43 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() fragd have to patch both kernels *in your case*. If it's all done by host, then it's in a single place, on host. id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. Why would non-gso frames need a frag id? We are talking only UDP IPv6 here, so there is no frag id generation if the packet does't need to be fragmented. I think that is the same reasoning why we don't support TOE. If we use one generator in the hypervisor in an openstack alike setting, the host deals with quite a lot of overlay networks. A lot of default configurations use the same addresses internally, so on the hypervisor the frag id generators would interfere by design. I could come up with an attack scenario for DNS servers (again :) ): You are sitting next to a DNS server on the same hypervisor and can send packets without source validation (because that is handled later on in case of openvswitch when the packet is put into the corresponding overlay network). You emit a gso packet with the same source
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. Confused. You would have to patch both kernels *in your case*. If it's all done by host, then it's in a single place, on host. I think that is the same reasoning why we don't support TOE. If we use one generator in the hypervisor in an openstack alike setting, the host
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. I think that is the same reasoning why we don't support TOE. If we use one generator in the hypervisor in an openstack alike setting, the host deals with quite a lot of overlay networks. A lot of default configurations use the same addresses internally, so on the hypervisor the frag id generators would interfere by design. I could come up with an attack scenario for DNS servers (again :) ): You are sitting next to a DNS server on the same hypervisor and can send packets without source validation (because that is handled later on in case of openvswitch when the packet is put into the corresponding overlay network). You emit a gso packet with the same source and destination addresses as the DNS server would do and would get an fragmentation id which is linearly (+ time delta) incremented depending on the source and destination address. With such a leak you could start trying attack and spoof DNS responses (fragmentation attacks etc.). See also details on such kind of attacks in the description of commit 04ca6973f7c1a0d. AFAIK IETF tried with IPv6 to push fragmentation id generation to the end hosts, that's also the reason for the introduction of atomic fragments
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. I think that is the same reasoning why we don't support TOE. If we use one generator in the hypervisor in an openstack alike setting, the host deals with quite a lot of overlay networks. A lot of default configurations use the same addresses internally, so on the hypervisor the frag id generators would interfere by design. I could come up with an attack scenario for DNS servers (again :) ): You are sitting next to a DNS server on the same hypervisor and can send packets without source validation (because that is handled later on in case of openvswitch when the packet is put into the corresponding overlay network). You emit a gso packet with the same source and destination addresses as the DNS server would do and would get an fragmentation id which is linearly (+ time delta) incremented depending on the source and destination address. With
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. I think that is the same reasoning why we don't support TOE. If we use one generator in the hypervisor in an openstack alike setting, the host deals with quite a lot of overlay networks. A lot of default configurations use the same addresses internally, so on the hypervisor the frag id generators would interfere by design. I could come up with an attack scenario for DNS servers (again :) ): You are sitting next to a DNS server on the same
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Mi, 2015-01-28 at 18:48 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 05:15:49PM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 18:00 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Wed, Jan 28, 2015 at 10:27:47AM -0500, Vlad Yasevich wrote: On 01/28/2015 09:45 AM, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 09:16 -0500, Vlad Yasevich wrote: On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. Why would non-gso frames need a frag id? We are talking only UDP IPv6 here, so there is no frag id generation if the packet does't need to be fragmented. E.g. raw sockets still can generate fragments locally. It is also a valid setup to have multiple interfaces in one machine, one that is UFO enabled and one that isn't. In that case, fragmentation id generation happens on different hosts which I want to avoid. OK, so you are concerned about both host and guest generating fragment
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
Hi, On Mi, 2015-01-28 at 09:16 -0500, Vlad Yasevich wrote: On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. Why would non-gso frames need a frag id? We are talking only UDP IPv6 here, so there is no frag id generation if the packet does't need to be fragmented. E.g. raw sockets still can generate fragments locally. It is also a valid setup to have multiple interfaces in one machine, one that is UFO enabled and one that isn't. In that case, fragmentation id generation happens on different hosts which I want to avoid. I haven't looked closely but mismatch of MTUs on interfaces seems like it could lead to unwanted fragmentation, e.g. see is_skb_forwardable which is mostly always true for gso frames, so we never stop them on bridges etc. I think that is the same
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. In that case doing things like extending virtio is out of the question too, isn't it? It needs hypervisor changes. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. I think that is the same reasoning why we don't support TOE. If we use one generator in the hypervisor in an openstack alike setting, the host deals with
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
Hi, On Mi, 2015-01-28 at 18:00 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. In that case doing things like extending virtio is out of the question too, isn't it? It needs hypervisor changes. Sure, but I would like to have the fragmentation id generator to reside inside the end-host kernel. Hypervisor
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On 01/28/2015 09:45 AM, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 09:16 -0500, Vlad Yasevich wrote: On 01/28/2015 05:34 AM, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my control. You would need to patch both kernels in your case - non gso frames would still get the fragmentation id generated in the host kernel. Why would non-gso frames need a frag id? We are talking only UDP IPv6 here, so there is no frag id generation if the packet does't need to be fragmented. E.g. raw sockets still can generate fragments locally. It is also a valid setup to have multiple interfaces in one machine, one that is UFO enabled and one that isn't. In that case, fragmentation id generation happens on different hosts which I want to avoid. OK, so you are concerned about both host and guest generating fragment ids. Host would do it for GSO frames and guest would do it for fragmented frames. Yes, there is room for collision, which is why we are aiming to fix this with fragment id passing through virtio_net. However, I am still trying to
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Wed, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: [...] I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. [...] You are advocating that the hypervisor should continue to act as a middle-box that quietly modifies packets. This may be useful to protect guests that have poor fragment ID generation, but then that should be an optional netfilter module and *not* the default. The default should be that UFO has no effect on the packet headers on the wire, and therefore that the fragment ID is chosen by the IPv6 stack in the guest. Ben. -- Ben Hutchings Teamwork is essential - it allows you to blame someone else. signature.asc Description: This is a digitally signed message part ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Wed, Jan 28, 2015 at 05:15:49PM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 18:00 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 11:34:02AM +0100, Hannes Frederic Sowa wrote: Hi, On Mi, 2015-01-28 at 11:46 +0200, Michael S. Tsirkin wrote: On Wed, Jan 28, 2015 at 09:25:08AM +0100, Hannes Frederic Sowa wrote: Hello, On Di, 2015-01-27 at 18:08 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. It is not only about entropy but about uniqueness. Also fragmentation ids should not be discoverable, I belive predictable is the language used by the IETF draft. so there are several aspects: I see fragmentation id generation still as security critical: When Eric patched the frag id generator in 04ca6973f7c1a0d (ip: make IP identifiers less predictable) I could patch my kernels and use the patch regardless of the machine being virtualized or not. It was not dependent on the hypervisor. And now it's even easier - just patch the hypervisor, and all VMs automatically benefit. Sometimes the hypervisor is not under my
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. -- Ben Hutchings When in doubt, use brute force. - Ken Thompson ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. Thanks, Hannes ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
From: Ben Hutchings b...@decadent.org.uk Date: Tue, 27 Jan 2015 02:47:54 + On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com Applied, thanks Ben. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. -vlad Thanks, Hannes ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Tue, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. With 32bit ID, you certainly can replace fragid=0 by fragid=0x8000 and nobody will notice. I certainly vote for not adding an extra bit in skb or skb_shared_info, considering we already consume 32bits for this thing. fragid are best effort, otherwise they would have 128bits. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Tue, Jan 27, 2015 at 05:02:31PM +0100, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes I'm not sure - what will be achieved by generating the IDs guest side as opposed to host side? It's certainly harder to get hold of entropy guest-side. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ Thoughts? Bye, Hannes ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On 01/27/2015 11:02 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 09:26 -0500, Vlad Yasevich wrote: On 01/27/2015 08:47 AM, Hannes Frederic Sowa wrote: On Di, 2015-01-27 at 10:42 +0200, Michael S. Tsirkin wrote: On Tue, Jan 27, 2015 at 02:47:54AM +, Ben Hutchings wrote: On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; -/* 3 or 5 bit hole */ +__u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. Hmm we seem to be out of tx flags. Maybe ip6_frag_id == 0 should mean not set. Maybe that is the best idea. Definitely the ufo_fragid_set bit should move into the skb_shared_info area. That's what I originally wanted to do, but had to move and grow txflags thus skb_shinfo ended up growing. I wanted to avoid that, so stole an skb flag. I considered treating fragid == 0 as unset, but a 0 fragid is perfectly valid from the protocol perspective and could actually be generated by the id generator functions. This may cause us to call the id generation multiple times. Are there plans in the long run to let virtio_net transmit auxiliary data to the other end so we can clean all of this this up one day? Yes, and I am working on this. Part of that is UFO/UFO6 split so that the fragment id is carried for UFO6. I don't like the whole situation: looking into the virtio_net headers just adding a field for ipv6 fragmentation ids to those small structs seems bloated, not doing it feels incorrect. :/ We are thinking right now how to extend virtio_net header as this is not the only extension people have thought off. It'll probably only happen for virtio 1.0 spec, so we may still have to support legacy devices that may still rely on UFO being available. -vlad Thoughts? Bye, Hannes ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.
On Mon, 2015-01-26 at 09:37 -0500, Vladislav Yasevich wrote: If the IPv6 fragment id has not been set and we perform fragmentation due to UFO, select a new fragment id. When we store the fragment id into skb_shinfo, set the bit in the skb so we can re-use the selected id. This preserves the behavior of UFO packets generated on the host and solves the issue of id generation for packet sockets and tap/macvtap devices. This patch moves ipv6_select_ident() back in to the header file. It also provides the helper function that sets skb_shinfo() frag id and sets the bit. It also makes sure that we select the fragment id when doing just gso validation, since it's possible for the packet to come from an untrusted source (VM) and be forwarded through a UFO enabled device which will expect the fragment id. CC: Eric Dumazet eduma...@google.com Signed-off-by: Vladislav Yasevich vyase...@redhat.com --- include/linux/skbuff.h | 3 ++- include/net/ipv6.h | 2 ++ net/ipv6/ip6_output.c | 4 ++-- net/ipv6/output_core.c | 9 - net/ipv6/udp_offload.c | 10 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 85ab7d7..3ad5203 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -605,7 +605,8 @@ struct sk_buff { __u8ipvs_property:1; __u8inner_protocol_type:1; __u8remcsum_offload:1; - /* 3 or 5 bit hole */ + __u8ufo_fragid_set:1; [...] Doesn't the flag belong in struct skb_shared_info, rather than struct sk_buff? Otherwise this looks fine. Ben. -- Ben Hutchings When in doubt, use brute force. - Ken Thompson signature.asc Description: This is a digitally signed message part ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization