linux-kernel - Re: [PATCH v2] net: dsa: tag_mtk: add padding for tx packets

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:   Thu, 12 May 2022 10:51:03 +0200
From:   Felix Fietkau <nbd@....name>
To:     Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>
Cc:     Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: dsa: tag_mtk: add padding for tx packets


On 11.05.22 16:32, Andrew Lunn wrote:
>> Let's see what others have to say. I've been wanting to make the policy
>> of whether to call __skb_put_padto() standardized for all tagging protocol
>> drivers (similar to what is done in dsa_realloc_skb() and below it).
>> We pad for tail taggers, maybe we can always pad and this removes a
>> conditional, and simplifies taggers. Side note, I already dislike that
>> the comment in tag_brcm.c is out of sync with the code. It says that
>> padding up to ETH_ZLEN is necessary, but proceeds to pad up until
>> ETH_ZLEN + tag len, only to add the tag len once more below via skb_push().
>> It would be nice if we could use the simple eth_skb_pad().
> 
> There are some master devices which will perform padding on their own,
> in hardware. So for taggers which insert the header at the head,
> forcing such padding would be a waste of CPU time.
> 
> For tail taggers, padding short packets by default does however make
> sense. The master device is probably going to pad in the wrong way if
> it does padding.
I just ran some more tests, here's what I found:
The switch automatically pads all forwarded packets to 64 bytes.
When packets are forwarded from one external port to another, the 
padding is all zero.
Only when packets are sent from a CPU port to an external port, the last 
4 bytes contain garbage. The garbage bytes are different for every 
packet, and I can't tell if it's leaking contents of previous packets or 
what else is in there.
Based on that, I'm pretty sure that the hardware simply has a quirk 
where it does not account for the special tag when generating its own 
padding internally.

I found that replacing my __skb_put_padto call with eth_skb_pad also 
works, so I'm going to send v3 with that and an updated comment.

- Felix