Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 May 2022 12:32:45 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Felix Fietkau <nbd@nbd.name>
Cc:     Sean Wang <sean.wang@mediatek.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        DENG Qingfang <dqfext@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: dsa: tag_mtk: add padding for tx packets
Message-ID: <20220511093245.3266lqdze2b4odh5@skbuf>
References: <20220510094014.68440-1-nbd@nbd.name>
 <20220510123724.i2xqepc56z4eouh2@skbuf>
 <5959946d-1d34-49b9-1abe-9f9299cc194e@nbd.name>
 <20220510165233.yahsznxxb5yq6rai@skbuf>
 <bc4bde22-c2d6-1ded-884a-69465b9d1dc7@nbd.name>
 <20220510222101.od3n7gk3cofwhbks@skbuf>
 <376b13ac-d90b-24e0-37ed-a96d8e5f80da@nbd.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <376b13ac-d90b-24e0-37ed-a96d8e5f80da@nbd.name>
Precedence: bulk

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.