netdev - Re: DSA and skb->protocol

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] [day] [month] [year] [list]

Message-ID: <541743F0.4090604@gmail.com>
Date:	Mon, 15 Sep 2014 12:54:24 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Andrew Lunn <andrew@...n.ch>,
	Alexander Duyck <alexander.h.duyck@...el.com>
CC:	seugene@...vell.com, netdev@...r.kernel.org
Subject: Re: DSA and skb->protocol

On 09/15/2014 11:32 AM, Andrew Lunn wrote:
>>> making me think it is not actually needed to change
>>> skb->protocol. When i remove this from edsa_xmit(), the warning goes
>>> away and networking seems to work O.K.
>>>
>>> Is this the right fix? Should we remove this setting of skb->protocol
>>> in the other dsa xmit functions?
>>>
>>> Thanks
>>> 	Andrew
>>>
>>
>> No, if anything they should all be using ETH_P_XDSA to indicate the
>> protocol between the switch and the host network interface.  When you
>> triggered this issue did you by any chance change some of the settings
>> on the host netdev for the switch?
> 
> This is the first time i've used DSA. I'm adding support for a new
> board which has a switch. So it is not too easy for me to go backwards
> and see when the WARNING started appearing.

We probably need something like a fix_feature() callback and/or a
NETDEV_FEAT_CHANGE() notifier callback to make sure we properly
recompute the slave network devices features based on the master network
device.

>  
>> The fix would probably be to update skb_network_protocol so that it can
>> sort out ETH_P_XDSA tags and get the Ethertype from the frame.
> 
> I'm not sure how easy getting the correct Ethertype from the frame
> is. The DSA tag can go in different places, depending on what tagging
> protocol is used.
> 
> For EDSA, which is what the board/switch i'm using uses, the tag is
> placed between the source address and the ethertype. If the frame is
> not an 802.1q, the tag is 8 bytes. If it is 802.1q the tag is 6
> bytes. The first 2 bytes are an Ethertype, indicating EDSA is being
> used. You need to look at byte 4 of the tag to know how long it is, in
> order to find the second Ethertype in the frame, for the SDU.
> 
> Similarly DSA tagging, is either 2 bytes or 4 bytes, and you need to
> look at byte 0 of the tag to determine its length. There is no
> Ethertype to know that DSA is being used.
> 
> Trailer tagging is different. It places 4 bytes at the end of the
> frame, applying padding for frames < 60 bytes. The Ethertype is not
> touched.
> 
> Florian recently added brcm tagging. This again goes between the
> Source address and the Ethertype, and does not define a valid
> Ethertype.
> 
> So unless you know what tagging protocol is in use, it is very hard to
> peer inside the frame to work out what the real SDU Ethertype is. So
> it seems easier to just pass it in skb->protocol.

Right, which is probably why the tag xmit() functions do override
skb->protocol to directly provide a value to the master device driver,
that way we do not have to go deep down into dissecting the frame.

I am not sure if there are possible scenarios where we may have to
handle mixed EDSA+DSA traffic, I suppose that might exist with a
combination of Marvell switches. If that's the case, we still need a
per-SKB information as opposed to looking directly into dev->dsa_ptr.
--
Florian
--
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