netdev - Re: [PATCH 1/3] ipv6: Select fragment id during UFO/GSO segmentation if not set.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Jan 2015 11:25:52 -0500
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
CC:	"Michael S. Tsirkin" <mst@...hat.com>,
	Ben Hutchings <ben@...adent.org.uk>,
	Vladislav Yasevich <vyasevich@...il.com>,
	netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org,
	edumazet@...gle.com
Subject: 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 +0000, 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 <edumazet@...gle.com>
>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@...hat.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 {
>>>>>>  	__u8			ipvs_property:1;
>>>>>>  	__u8			inner_protocol_type:1;
>>>>>>  	__u8			remcsum_offload:1;
>>>>>> -	/* 3 or 5 bit hole */
>>>>>> +	__u8			ufo_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
> 
> 

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