[PATCH v2] net: dsa: tag_mtk: add padding for tx packets

[PATCH v2] net: dsa: tag_mtk: add padding for tx packets

[Felix Fietkau]

Padding for transmitted packets needs to account for the special tag.
With not enough padding, garbage bytes are inserted by the switch at the
end of small packets.

Fixes: 5cd8985a1909 ("net-next: dsa: add Mediatek tag RX/TX handler")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/dsa/tag_mtk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 415d8ece242a..1d1f9dbd9e93 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -25,6 +25,14 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	u8 xmit_tpid;
 	u8 *mtk_tag;
 
+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes (including FCS) otherwise their padding might be
+	 * corrupted. With tags enabled, we need to make sure that packets are
+	 * at least 68 bytes (including FCS and tag).
+	 */
+	if (__skb_put_padto(skb, ETH_ZLEN + MTK_HDR_LEN, false))
+		return NULL;
+
 	/* Build the special tag after the MAC Source Address. If VLAN header
 	 * is present, it's required that VLAN header and special tag is
 	 * being combined. Only in this way we can allow the switch can parse
-- 
2.36.1

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

[Vladimir Oltean]

On Tue, May 10, 2022 at 11:40:13AM +0200, Felix Fietkau wrote:
> Padding for transmitted packets needs to account for the special tag.
> With not enough padding, garbage bytes are inserted by the switch at the
> end of small packets.

I don't think padding bytes are guaranteed to be zeroes. Aren't they
discarded? What is the issue?

> 
> Fixes: 5cd8985a1909 ("net-next: dsa: add Mediatek tag RX/TX handler")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/dsa/tag_mtk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> index 415d8ece242a..1d1f9dbd9e93 100644
> --- a/net/dsa/tag_mtk.c
> +++ b/net/dsa/tag_mtk.c
> @@ -25,6 +25,14 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>  	u8 xmit_tpid;
>  	u8 *mtk_tag;
>  
> +	/* The Ethernet switch we are interfaced with needs packets to be at
> +	 * least 64 bytes (including FCS) otherwise their padding might be
> +	 * corrupted. With tags enabled, we need to make sure that packets are
> +	 * at least 68 bytes (including FCS and tag).
> +	 */
> +	if (__skb_put_padto(skb, ETH_ZLEN + MTK_HDR_LEN, false))
> +		return NULL;
> +
>  	/* Build the special tag after the MAC Source Address. If VLAN header
>  	 * is present, it's required that VLAN header and special tag is
>  	 * being combined. Only in this way we can allow the switch can parse
> -- 
> 2.36.1
> 

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

[Felix Fietkau]

On 10.05.22 14:37, Vladimir Oltean wrote:
> On Tue, May 10, 2022 at 11:40:13AM +0200, Felix Fietkau wrote:
>> Padding for transmitted packets needs to account for the special tag.
>> With not enough padding, garbage bytes are inserted by the switch at the
>> end of small packets.
> 
> I don't think padding bytes are guaranteed to be zeroes. Aren't they
> discarded? What is the issue?
With the broken padding, ARP requests are silently discarded on the 
receiver side in my test. Adding the padding explicitly fixes the issue.

- Felix

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

[Vladimir Oltean]

On Tue, May 10, 2022 at 04:52:16PM +0200, Felix Fietkau wrote:
> 
> On 10.05.22 14:37, Vladimir Oltean wrote:
> > On Tue, May 10, 2022 at 11:40:13AM +0200, Felix Fietkau wrote:
> > > Padding for transmitted packets needs to account for the special tag.
> > > With not enough padding, garbage bytes are inserted by the switch at the
> > > end of small packets.
> > 
> > I don't think padding bytes are guaranteed to be zeroes. Aren't they
> > discarded? What is the issue?
> With the broken padding, ARP requests are silently discarded on the receiver
> side in my test. Adding the padding explicitly fixes the issue.
> 
> - Felix

Ok, I'm not going to complain too much about the patch, but I'm still
curious where are the so-called "broken" packets discarded.
I think the receiving MAC should be passing up to software a buffer
without the extra padding beyond the L2 payload length (at least that's
the behavior I'm familiar with).

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

[Felix Fietkau]

On 10.05.22 18:52, Vladimir Oltean wrote:
> On Tue, May 10, 2022 at 04:52:16PM +0200, Felix Fietkau wrote:
>> 
>> On 10.05.22 14:37, Vladimir Oltean wrote:
>> > On Tue, May 10, 2022 at 11:40:13AM +0200, Felix Fietkau wrote:
>> > > Padding for transmitted packets needs to account for the special tag.
>> > > With not enough padding, garbage bytes are inserted by the switch at the
>> > > end of small packets.
>> > 
>> > I don't think padding bytes are guaranteed to be zeroes. Aren't they
>> > discarded? What is the issue?
>> With the broken padding, ARP requests are silently discarded on the receiver
>> side in my test. Adding the padding explicitly fixes the issue.
>> 
>> - Felix
> 
> Ok, I'm not going to complain too much about the patch, but I'm still
> curious where are the so-called "broken" packets discarded.
> I think the receiving MAC should be passing up to software a buffer
> without the extra padding beyond the L2 payload length (at least that's
> the behavior I'm familiar with).

I don't know where exactly these packets are discarded.
After digging through the devices I used during the tests, I just found 
some leftover pcap files that show the differences in the received 
packets. Since the packets are bigger after my patch, I can't rule out 
that packet size instead of the padding may have made a difference here 
in getting the ARP requests accepted by the receiver.

I've extracted the ARP requests and you can find them here:
http://nbd.name/arp-broken.pcap
http://nbd.name/arp-working.pcap

- Felix

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

[Vladimir Oltean]

On Wed, May 11, 2022 at 12:06:25AM +0200, Felix Fietkau wrote:
>
> On 10.05.22 18:52, Vladimir Oltean wrote:
> > On Tue, May 10, 2022 at 04:52:16PM +0200, Felix Fietkau wrote:
> > >
> > > On 10.05.22 14:37, Vladimir Oltean wrote:
> > > > On Tue, May 10, 2022 at 11:40:13AM +0200, Felix Fietkau wrote:
> > > > > Padding for transmitted packets needs to account for the special tag.
> > > > > With not enough padding, garbage bytes are inserted by the switch at the
> > > > > end of small packets.
> > > > > I don't think padding bytes are guaranteed to be zeroes. Aren't
> > > they
> > > > discarded? What is the issue?
> > > With the broken padding, ARP requests are silently discarded on the receiver
> > > side in my test. Adding the padding explicitly fixes the issue.
> > >
> > > - Felix
> >
> > Ok, I'm not going to complain too much about the patch, but I'm still
> > curious where are the so-called "broken" packets discarded.
> > I think the receiving MAC should be passing up to software a buffer
> > without the extra padding beyond the L2 payload length (at least that's
> > the behavior I'm familiar with).
>
> I don't know where exactly these packets are discarded.
> After digging through the devices I used during the tests, I just found some
> leftover pcap files that show the differences in the received packets. Since
> the packets are bigger after my patch, I can't rule out that packet size
> instead of the padding may have made a difference here in getting the ARP
> requests accepted by the receiver.
>
> I've extracted the ARP requests and you can find them here:
> http://nbd.name/arp-broken.pcap

arp-broken.pcap was collected at the receiver MAC, right? So the packets
actually exited the switch?

> http://nbd.name/arp-working.pcap
>
> - Felix

It sounds as if this is masking a problem on the receiver end, because
not only does my enetc port receive the packet, it also replies to the
ARP request.

pc # sudo tcpreplay -i eth1 arp-broken.pcap
root@debian:~# ip addr add 192.168.42.1/24 dev eno0
root@debian:~# tcpdump -i eno0 -e -n --no-promiscuous-mode arp
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on eno0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
22:18:58.846753 f4:d4:88:5e:6f:d2 > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 60: Request who-has 192.168.42.1 tell 192.168.42.173, length 46
22:18:58.846806 00:04:9f:05:f4:ab > f4:d4:88:5e:6f:d2, ethertype ARP (0x0806), length 42: Reply 192.168.42.1 is-at 00:04:9f:05:f4:ab, length 28
^C
2 packets captured
2 packets received by filter
0 packets dropped by kernel

What MAC/driver has trouble with these packets? Is there anything wrong
in ethtool stats? Do they even reach software? You can also use
"dropwatch -l kas" for some hints if they do.

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

[Felix Fietkau]

Hi Vladimir,


On 11.05.22 00:21, Vladimir Oltean wrote:
> It sounds as if this is masking a problem on the receiver end, because
> not only does my enetc port receive the packet, it also replies to the
> ARP request.
> 
> pc # sudo tcpreplay -i eth1 arp-broken.pcap
> root@debian:~# ip addr add 192.168.42.1/24 dev eno0
> root@debian:~# tcpdump -i eno0 -e -n --no-promiscuous-mode arp
> tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
> listening on eno0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
> 22:18:58.846753 f4:d4:88:5e:6f:d2 > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 60: Request who-has 192.168.42.1 tell 192.168.42.173, length 46
> 22:18:58.846806 00:04:9f:05:f4:ab > f4:d4:88:5e:6f:d2, ethertype ARP (0x0806), length 42: Reply 192.168.42.1 is-at 00:04:9f:05:f4:ab, length 28
> ^C
> 2 packets captured
> 2 packets received by filter
> 0 packets dropped by kernel
> 
> What MAC/driver has trouble with these packets? Is there anything wrong
> in ethtool stats? Do they even reach software? You can also use
> "dropwatch -l kas" for some hints if they do.
For some reason I can't reproduce the issue of ARPs not getting replies 
anymore.
The garbage data is still present in the ARP packets without my patch 
though. So regardless of whether ARP packets are processed correctly or 
if they just trip up on some receivers under specific conditions, I 
believe my patch is valid and should be applied.

Who knows, maybe the garbage padding even leaks some data from previous 
packets, or some other information from within the switch.

- Felix

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

[Vladimir Oltean]

On Wed, May 11, 2022 at 10:50:17AM +0200, Felix Fietkau wrote:
> Hi Vladimir,
> 
> On 11.05.22 00:21, Vladimir Oltean wrote:
> > It sounds as if this is masking a problem on the receiver end, because
> > not only does my enetc port receive the packet, it also replies to the
> > ARP request.
> > 
> > pc # sudo tcpreplay -i eth1 arp-broken.pcap
> > root@debian:~# ip addr add 192.168.42.1/24 dev eno0
> > root@debian:~# tcpdump -i eno0 -e -n --no-promiscuous-mode arp
> > tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
> > listening on eno0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
> > 22:18:58.846753 f4:d4:88:5e:6f:d2 > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 60: Request who-has 192.168.42.1 tell 192.168.42.173, length 46
> > 22:18:58.846806 00:04:9f:05:f4:ab > f4:d4:88:5e:6f:d2, ethertype ARP (0x0806), length 42: Reply 192.168.42.1 is-at 00:04:9f:05:f4:ab, length 28
> > ^C
> > 2 packets captured
> > 2 packets received by filter
> > 0 packets dropped by kernel
> > 
> > What MAC/driver has trouble with these packets? Is there anything wrong
> > in ethtool stats? Do they even reach software? You can also use
> > "dropwatch -l kas" for some hints if they do.
> 
> For some reason I can't reproduce the issue of ARPs not getting replies
> anymore.
> The garbage data is still present in the ARP packets without my patch
> though. So regardless of whether ARP packets are processed correctly or if
> they just trip up on some receivers under specific conditions, I believe my
> patch is valid and should be applied.

I don't have a very strong opinion regarding whether to apply the patch or not.
I think we've removed it from bug fix territory now, until proven otherwise.
I do care about the justification (commit message, comments) being
correct though. If you cannot reproduce now, someone one year from now
surely cannot reproduce it either, and won't know why the code is there.
FYI, the reason why you call __skb_put_padto() is not the reason why
others call __skb_put_padto().

> Who knows, maybe the garbage padding even leaks some data from previous
> packets, or some other information from within the switch.

I mean, the padding has to come from somewhere, no? Although I'd
probably imagine non-scrubbed buffer cells rather than data structures...

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().

But there will be a small performance degradation for small packets due
to the memset in __skb_pad(), which I'm not sure is worth the change.

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

[Felix Fietkau]

On 11.05.22 11:32, Vladimir Oltean wrote:
> On Wed, May 11, 2022 at 10:50:17AM +0200, Felix Fietkau wrote:
>> Hi Vladimir,
>> 
>> On 11.05.22 00:21, Vladimir Oltean wrote:
>> > It sounds as if this is masking a problem on the receiver end, because
>> > not only does my enetc port receive the packet, it also replies to the
>> > ARP request.
>> > 
>> > pc # sudo tcpreplay -i eth1 arp-broken.pcap
>> > root@debian:~# ip addr add 192.168.42.1/24 dev eno0
>> > root@debian:~# tcpdump -i eno0 -e -n --no-promiscuous-mode arp
>> > tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
>> > listening on eno0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
>> > 22:18:58.846753 f4:d4:88:5e:6f:d2 > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 60: Request who-has 192.168.42.1 tell 192.168.42.173, length 46
>> > 22:18:58.846806 00:04:9f:05:f4:ab > f4:d4:88:5e:6f:d2, ethertype ARP (0x0806), length 42: Reply 192.168.42.1 is-at 00:04:9f:05:f4:ab, length 28
>> > ^C
>> > 2 packets captured
>> > 2 packets received by filter
>> > 0 packets dropped by kernel
>> > 
>> > What MAC/driver has trouble with these packets? Is there anything wrong
>> > in ethtool stats? Do they even reach software? You can also use
>> > "dropwatch -l kas" for some hints if they do.
>> 
>> For some reason I can't reproduce the issue of ARPs not getting replies
>> anymore.
>> The garbage data is still present in the ARP packets without my patch
>> though. So regardless of whether ARP packets are processed correctly or if
>> they just trip up on some receivers under specific conditions, I believe my
>> patch is valid and should be applied.
> 
> I don't have a very strong opinion regarding whether to apply the patch or not.
> I think we've removed it from bug fix territory now, until proven otherwise.
I strongly disagree. Without my fix we're relying on undefined behavior 
of the hardware, since the switch requires padding that accounts for the 
special tag.

> I do care about the justification (commit message, comments) being
> correct though. If you cannot reproduce now, someone one year from now
> surely cannot reproduce it either, and won't know why the code is there.
I think there is some misunderstanding here. I absolutely can reproduce 
the corrupted padding reliably, and it matches what I put into commit 
message and comments.

The issue that I can't reproduce reliably at the moment (ARP reception 
failure) is something that I only pointed out in a reply to this thread.
This is what prompted me to look into the padding issue in the first 
place, and it also matches reports about connectivity issues that I got 
from other people.

> FYI, the reason why you call __skb_put_padto() is not the reason why
> others call __skb_put_padto().
It matches the call in tag_brcm.c (because I copied it from there), it's 
just that the symptoms that I'm fixing are different (undefined behavior 
instead of hard packet drop in the switch logic).

>> Who knows, maybe the garbage padding even leaks some data from previous
>> packets, or some other information from within the switch.
> 
> I mean, the padding has to come from somewhere, no? Although I'd
> probably imagine non-scrubbed buffer cells rather than data structures...
> 
> 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().
> 
> But there will be a small performance degradation for small packets due
> to the memset in __skb_pad(), which I'm not sure is worth the change.
I guess we have different views on this. In my opinion, correctness 
matters more in this case than the tiny performance degradation.

- Felix

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

[Vladimir Oltean]

On Wed, May 11, 2022 at 02:24:19PM +0200, Felix Fietkau wrote:
> 
> On 11.05.22 11:32, Vladimir Oltean wrote:
> > On Wed, May 11, 2022 at 10:50:17AM +0200, Felix Fietkau wrote:
> > > Hi Vladimir,
> > > 
> > > On 11.05.22 00:21, Vladimir Oltean wrote:
> > > > It sounds as if this is masking a problem on the receiver end, because
> > > > not only does my enetc port receive the packet, it also replies to the
> > > > ARP request.
> > > > > pc # sudo tcpreplay -i eth1 arp-broken.pcap
> > > > root@debian:~# ip addr add 192.168.42.1/24 dev eno0
> > > > root@debian:~# tcpdump -i eno0 -e -n --no-promiscuous-mode arp
> > > > tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
> > > > listening on eno0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
> > > > 22:18:58.846753 f4:d4:88:5e:6f:d2 > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 60: Request who-has 192.168.42.1 tell 192.168.42.173, length 46
> > > > 22:18:58.846806 00:04:9f:05:f4:ab > f4:d4:88:5e:6f:d2, ethertype ARP (0x0806), length 42: Reply 192.168.42.1 is-at 00:04:9f:05:f4:ab, length 28
> > > > ^C
> > > > 2 packets captured
> > > > 2 packets received by filter
> > > > 0 packets dropped by kernel
> > > > > What MAC/driver has trouble with these packets? Is there
> > > anything wrong
> > > > in ethtool stats? Do they even reach software? You can also use
> > > > "dropwatch -l kas" for some hints if they do.
> > > 
> > > For some reason I can't reproduce the issue of ARPs not getting replies
> > > anymore.
> > > The garbage data is still present in the ARP packets without my patch
> > > though. So regardless of whether ARP packets are processed correctly or if
> > > they just trip up on some receivers under specific conditions, I believe my
> > > patch is valid and should be applied.
> > 
> > I don't have a very strong opinion regarding whether to apply the patch or not.
> > I think we've removed it from bug fix territory now, until proven otherwise.
> 
> I strongly disagree. Without my fix we're relying on undefined behavior of
> the hardware, since the switch requires padding that accounts for the
> special tag.
> 
> > I do care about the justification (commit message, comments) being
> > correct though. If you cannot reproduce now, someone one year from now
> > surely cannot reproduce it either, and won't know why the code is there.
> 
> I think there is some misunderstanding here. I absolutely can reproduce the
> corrupted padding reliably, and it matches what I put into commit message
> and comments.
> 
> The issue that I can't reproduce reliably at the moment (ARP reception
> failure) is something that I only pointed out in a reply to this thread.
> This is what prompted me to look into the padding issue in the first place,
> and it also matches reports about connectivity issues that I got from other
> people.
> 
> > FYI, the reason why you call __skb_put_padto() is not the reason why
> > others call __skb_put_padto().
> 
> It matches the call in tag_brcm.c (because I copied it from there), it's
> just that the symptoms that I'm fixing are different (undefined behavior
> instead of hard packet drop in the switch logic).
> 
> > > Who knows, maybe the garbage padding even leaks some data from previous
> > > packets, or some other information from within the switch.
> > 
> > I mean, the padding has to come from somewhere, no? Although I'd
> > probably imagine non-scrubbed buffer cells rather than data structures...
> > 
> > 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().
> > 
> > But there will be a small performance degradation for small packets due
> > to the memset in __skb_pad(), which I'm not sure is worth the change.
> 
> I guess we have different views on this. In my opinion, correctness matters
> more in this case than the tiny performance degradation.

It seems that we are in disagreement about what it is that I am disputing.
I am not disputing that the switch inserts non-zero padding octets, I am
disputing the claim that this somehow violates any standard.

IEEE 802.3 clause 4.2.8 Frame transmission details this beyond any doubt.
It says:

ComputePad appends an array of arbitrary bits to the MAC client data to
pad the frame to the minimum frame size:

function ComputePad(var dataParam: DataValue): DataValue;
begin
    ComputePad := {Append an array of size padSize of arbitrary bits to the MAC client dataField}
end; {ComputePad}

Clause 4.2.9 Frame reception then proceeds to say (too long to copy it,
sorry) that the RemovePad function truncates the dataParam when possible
(which has to do with whether the FCS is passed up to software or not)
to the value represented by the lengthOrTypeParam (in octets), therefore
*not* looking at the contents of the padding (other than to validate the
FCS).

So, what correctness are we talking about?

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

[Andrew Lunn]

> 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.

   Andrew

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

[Felix Fietkau]

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

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

[Andrew Lunn]

Hi Felix

Thanks for the additional testing.

> 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.

This does not yet explain why your receiver is dropping the frame. As
Vladimir pointed out, the contents of the pad should not matter.

Is it also getting the FCS wrong when it pads? That would cause the
receiver to drop the frame.

Or do we have an issue in the receiver where it is looking at the
contents of the pad?

	 Andrew

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

[Felix Fietkau]

Hi Andrew,

On 12.05.22 14:39, Andrew Lunn wrote:
>> 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.
> 
> This does not yet explain why your receiver is dropping the frame. As
> Vladimir pointed out, the contents of the pad should not matter.
> 
> Is it also getting the FCS wrong when it pads? That would cause the
> receiver to drop the frame.
> 
> Or do we have an issue in the receiver where it is looking at the
> contents of the pad?
On the devices that I used for testing before, FCS wasn't reported in my 
captures. Since I can't reproduce the issue of the receiver dropping 
frames anymore, I currently have no way of figuring out what went wrong.

When I was able to reproduce the issue, I'm sure that I switched between 
patched and unpatched builds a few times to make sure that my change 
actually made a difference, which it did.

I do agree that having the garbage bytes in there is technically 
compliant with the spec. On the other hand, based on my observations I 
believe that the hardware's behavior of filling the last 4 bytes with 
seemingly random values only in the case of small frames being sent with 
a CPU special tag is clearly not intentional nor by design.

The issue is also clearly limited to processing packets with the tag 
(which can only come from the CPU), so in my opinion the tag driver is 
the right place to deal with it.

So I guess it comes down to whether you guys think that this is worth 
fixing.

I still consider it worth fixing, because:
- It looks like a hardware bug to me with potentially unknown consequences.
- If it caused issues in my setup, it might do so in other people's 
setups as well.
- I can't rule out potential information leakage from those 4 bytes

I guess if you guys don't think the issue is worth the price of a very 
small performance hit from padding the packets, I will just have to keep 
this as an out-of-tree patch.

- Felix

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

[Andrew Lunn]

> I guess if you guys don't think the issue is worth the price of a very small
> performance hit from padding the packets, I will just have to keep this as
> an out-of-tree patch.

I'm thinking it is worth fixing, but i also wonder if your receiver
has a bug. And if it does have a bug, it will probably come back to
bite you sometime in the future.

     Andrew

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

[Andrew Lunn]

> The garbage data is still present in the ARP packets without my patch
> though. So regardless of whether ARP packets are processed correctly or if
> they just trip up on some receivers under specific conditions, I believe my
> patch is valid and should be applied.
> 
> Who knows, maybe the garbage padding even leaks some data from previous
> packets, or some other information from within the switch.

I somewhat agree with Vladimir at the moment. We don't seem to fully
understand why the change makes things work better. And without that
understanding, it is hard to say if this is the correct fix or not.

If this fix is making the transmitter work around bugs in the
receiver, we really should be fixing the receiver, otherwise the
receiver is going to be broken in other scenarios involving padded
short packets.

	 Andrew

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

[Jakub Kicinski]

On Tue, 10 May 2022 11:40:13 +0200 Felix Fietkau wrote:
> Padding for transmitted packets needs to account for the special tag.
> With not enough padding, garbage bytes are inserted by the switch at the
> end of small packets.
> 
> Fixes: 5cd8985a1909 ("net-next: dsa: add Mediatek tag RX/TX handler")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

You should answer all questions and address or dispute all feedback.