netdev - Re: [PATCH net-next v8 3/5] net: dsa: add out-of-band tagging 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <6b38ec27-65a3-c973-c5e1-a25bbe4f6104@nbd.name>
Date:   Tue, 8 Nov 2022 13:22:17 +0100
From:   Felix Fietkau <nbd@....name>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org,
        Vladimir Oltean <vladimir.oltean@....com>,
        Luka Perkov <luka.perkov@...tura.hr>,
        Robert Marko <robert.marko@...tura.hr>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>
Subject: Re: [PATCH net-next v8 3/5] net: dsa: add out-of-band tagging
 protocol

On 07.11.22 09:39, Maxime Chevallier wrote:
>> On Fri,  4 Nov 2022 18:41:49 +0100 Maxime Chevallier wrote:
>> > This tagging protocol is designed for the situation where the link
>> > between the MAC and the Switch is designed such that the Destination
>> > Port, which is usually embedded in some part of the Ethernet
>> > Header, is sent out-of-band, and isn't present at all in the
>> > Ethernet frame.
>> > 
>> > This can happen when the MAC and Switch are tightly integrated on an
>> > SoC, as is the case with the Qualcomm IPQ4019 for example, where
>> > the DSA tag is inserted directly into the DMA descriptors. In that
>> > case, the MAC driver is responsible for sending the tag to the
>> > switch using the out-of-band medium. To do so, the MAC driver needs
>> > to have the information of the destination port for that skb.
>> > 
>> > Add a new tagging protocol based on SKB extensions to convey the
>> > information about the destination port to the MAC driver  
>> 
>> This is what METADATA_HW_PORT_MUX is for, you shouldn't have 
>> to allocate a piece of memory for every single packet.
> 
> Does this work with DSA ? The information conveyed in the extension is
> the DSA port identifier. I'm not familiar at all with
> METADATA_HW_PORT_MUX, should we extend that mechanism to convey the
> DSA port id ?
> 
> I also agree that allocating data isn't the best way to go, but from
> the history of this series, we've tried 3 approaches so far :
> 
>   - Adding a new field to struct sk_buff, which isn't a good idea
>   - Using the skb headroom, but then we can't know for sure is the skb
>     contains a DSA tag or not
>   - Using skb extensions, that comes with the cost of this memory
>     allocation. Is this approach also incorrect then ?
FYI, I'm currently working on hardware DSA untagging on the mediatek
mtk_eth_soc driver. On this hardware, I definitely need to keep the
custom DSA tag driver, as hardware untagging is not always available.
For the receive side, I came up with this patch (still untested) for
using METADATA_HW_PORT_MUX.
It has the advantage of being able to skip the tag protocol rcv ops
call for offload-enabled packets.

Maybe for the transmit side we could have some kind of netdev feature
or capability that indicates offload support and allows skipping the
tag xmit function as well.
In that case, ipqess could simply use a no-op tag driver.

What do you think?

---
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -972,11 +972,13 @@ bool __skb_flow_dissect(const struct net *net,
  		if (unlikely(skb->dev && netdev_uses_dsa(skb->dev) &&
  			     skb->protocol == htons(ETH_P_XDSA))) {
  			const struct dsa_device_ops *ops;
+			struct metadata_dst *md_dst = skb_metadata_dst(skb);
  			int offset = 0;
  
  			ops = skb->dev->dsa_ptr->tag_ops;
  			/* Only DSA header taggers break flow dissection */
-			if (ops->needed_headroom) {
+			if (ops->needed_headroom &&
+			    (!md_dst || md_dst->type != METADATA_HW_PORT_MUX)) {
  				if (ops->flow_dissect)
  					ops->flow_dissect(skb, &proto, &offset);
  				else
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -11,6 +11,7 @@
  #include <linux/netdevice.h>
  #include <linux/sysfs.h>
  #include <linux/ptp_classify.h>
+#include <net/dst_metadata.h>
  
  #include "dsa_priv.h"
  
@@ -216,6 +217,7 @@ static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
  			  struct packet_type *pt, struct net_device *unused)
  {
+	struct metadata_dst *md_dst = skb_metadata_dst(skb);
  	struct dsa_port *cpu_dp = dev->dsa_ptr;
  	struct sk_buff *nskb = NULL;
  	struct dsa_slave_priv *p;
@@ -229,7 +231,21 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
  	if (!skb)
  		return 0;
  
-	nskb = cpu_dp->rcv(skb, dev);
+	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
+		unsigned int port = md_dst->u.port_info.port_id;
+
+		dsa_default_offload_fwd_mark(skb);
+		skb_dst_set(skb, NULL);
+		if (!skb_has_extensions(skb))
+			skb->slow_gro = 0;
+
+		skb->dev = dsa_master_find_slave(dev, 0, port);
+		if (skb->dev)
+			nskb = skb;
+	} else {
+		nskb = cpu_dp->rcv(skb, dev);
+	}
+
  	if (!nskb) {
  		kfree_skb(skb);
  		return 0;