netdev - Re: [PATCH] net: dsa: ksz: Increase the tag alignment

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]

Message-ID: <59899b2b-723b-44ec-6eb2-ff9d07e1102a@gmail.com>
Date:   Mon, 10 Dec 2018 13:39:04 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tristram.Ha@...rochip.com, marex@...x.de
Cc:     vivien.didelot@...oirfairelinux.com, andrew@...n.ch,
        Woojung.Huh@...rochip.com, UNGLinuxDriver@...rochip.com,
        davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] net: dsa: ksz: Increase the tag alignment

On 12/10/18 12:04 PM, Tristram.Ha@...rochip.com wrote:
>>>>>> -	padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len;
>>>>>> +	padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb-
>>>>>>> len;
>>> Oh so they add the internal VLAN at the end of the frame, not the
>>> beginning? That is quite surprising but that would not be the one single
>>> oddity found with CPSW I am afraid.. The way I would approach this is
>>> with layering where the padding needs to occur:
>>>
>>> - within the tag driver you need to make sure there is enough room to
>>> insert the KSZ tag
>>>
>>> - within the ethernet MAC driver (which comes last in the transmit
>>> path), you need to make sure there is enough room to insert that trailer
>>> VLAN tag to permit internal transmission
>>
>> So you think this is a bug in the CPSW instead ?
>>
> 
> I think what causes this problem.  In the MAC controller driver cpsw.c
> the buffer is always padded to CPSW_MIN_PACKET_SIZE.  Normally that
> size is 60 bytes, but after Linux 4.14 kernel it was changed to VLAN_ETH_ZLEN.
> The original size should work, but I do not know why it was changed.  It seems
> there is a new function using the CPSW_RX_VLAN_ENCAP bit.
> 
> It is similar to what I experienced with the Atmel MAC driver.  The newer kernels
> added some changes that introduced a bug and broke the tail tagging code.  I had
> to submit a fix to the MAC driver to correct that.

That is presumably this commit that you are referring to:

a52ad514fdf3b8a57ca4322c92d2d8d5c6182485 ("net: deprecate
eth_change_mtu, remove usage")?

> 
> I do not think this patch should apply generally, but I do not know how to fix the
> MAC driver to work in all cases.

If you find yourself fixing all possible MAC controllers to work with
the trailer code then this does not scale, and clearly we must find a
better way to advertise what minimum frame size is required, as well as
possible carry some information about how big that trailer tag must be.
Andrew recently did that by having the DSA code call back into the
Ethernet MAC driver to re-configure the MTU to account for how many
bytes the tagging code is going to need.

Keep in mind that one of the design paradigm of DSA is that because you
are typically dealing with external/discrete chips *ANY* Ethernet MAC
controller must be working with the trailer/header/tagging code. MAC
controllers typically require bytes somewhere at the front of the packet
(usually) to insert controller specific metadata, so eventually the MAC
driver is the best place to make sure the packet can successfully be
transmitted.

The main issue I see with trailer tags here is that if you have multiple
of them, despite knowing in which order they are going to be appended to
the frame, there is not enough metadata with the SKB to know where the
next level should be inserting the trailer, in that case, CPSW does not
know where to insert it because the KSZ trailer is already there and
skb->len has already reflected that increase in lengths. Maybe you need
to make use of skb->cb[] in that case to communicate where the trailer
was inserted?

> 
> You can try temporarily to change CPSW_MIN_PACKET_SIZE back to 60.
> 
> It is only used to assign to min_packet_size.  It may be possible to use a different
> size like 60 instead of 64 in the skb_padto function.
> 

An argument could be made that the stack should be passing up 60 bytes
(sans FCS) frames to the Ethernet driver already, instead of having each
Ethernet MAC controller have to decide how to manage RUNT packets, but
there are many code paths within the stack (including things like raw
sockets) that would be hard to fix. Also, each Ethernet MAC driver being
different, some are able to do HW padding of packets with 0, while some
require software to do it.

Anyway....
-- 
Florian