LinuxLists.cc - [PATCH net-next 0/9] Remove skb_mac_header() dependency in DSA xmit path

[permalink] [raw]

Subject: [PATCH net-next 2/9] net: vlan: introduce skb_vlan_eth_hdr()

Similar to skb_eth_hdr() introduced in commit 96cc4b69581d ("macvlan: do
not assume mac_header is set in macvlan_broadcast()"), let's introduce a
skb_vlan_eth_hdr() helper which can be used in TX-only code paths to get
to the VLAN header based on skb->data rather than based on the
skb_mac_header(skb).

We also consolidate the drivers that dereference skb->data to go through
this helper.

Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 3 +--
drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c | 2 +-
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 4 ++--
drivers/net/ethernet/sfc/tx_tso.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 ++-----
drivers/staging/gdm724x/gdm_lte.c | 4 ++--
include/linux/if_vlan.h | 12 ++++++++++--
net/batman-adv/soft-interface.c | 2 +-
12 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 16c490692f42..4950fde82d17 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -1923,8 +1923,7 @@ u16 bnx2x_select_queue(struct net_device *dev, struct sk_buff *skb,

/* Skip VLAN tag if present */
if (ether_type == ETH_P_8021Q) {
- struct vlan_ethhdr *vhdr =
- (struct vlan_ethhdr *)skb->data;
+ struct vlan_ethhdr *vhdr = skb_vlan_eth_hdr(skb);

ether_type = ntohs(vhdr->h_vlan_encapsulated_proto);
}
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index aed1b622f51f..7e408bcc88de 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -1124,7 +1124,7 @@ static struct sk_buff *be_lancer_xmit_workarounds(struct be_adapter *adapter,
struct be_wrb_params
*wrb_params)
{
- struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
+ struct vlan_ethhdr *veh = skb_vlan_eth_hdr(skb);
unsigned int eth_hdr_len;
struct iphdr *ip;

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 5caea154362f..7356ad965487 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -1532,7 +1532,7 @@ static int hns3_handle_vtags(struct hns3_enet_ring *tx_ring,
if (unlikely(rc < 0))
return rc;

- vhdr = (struct vlan_ethhdr *)skb->data;
+ vhdr = skb_vlan_eth_hdr(skb);
vhdr->h_vlan_TCI |= cpu_to_be16((skb->priority << VLAN_PRIO_SHIFT)
& VLAN_PRIO_MASK);

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 32cce90abbb4..81856f444d38 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -3063,7 +3063,7 @@ static inline int i40e_tx_prepare_vlan_flags(struct sk_buff *skb,
rc = skb_cow_head(skb, 0);
if (rc < 0)
return rc;
- vhdr = (struct vlan_ethhdr *)skb->data;
+ vhdr = skb_vlan_eth_hdr(skb);
vhdr->h_vlan_TCI = htons(tx_flags >>
I40E_TX_FLAGS_VLAN_SHIFT);
} else {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 773c35fecace..eae6c89e62f4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8818,7 +8818,7 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,

if (skb_cow_head(skb, 0))
goto out_drop;
- vhdr = (struct vlan_ethhdr *)skb->data;
+ vhdr = skb_vlan_eth_hdr(skb);
vhdr->h_vlan_TCI = htons(tx_flags >>
IXGBE_TX_FLAGS_VLAN_SHIFT);
} else {
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 59d0dd862fd1..1d1e183d3a8b 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -1854,7 +1854,7 @@ netxen_tso_check(struct net_device *netdev,

if (protocol == cpu_to_be16(ETH_P_8021Q)) {

- vh = (struct vlan_ethhdr *)skb->data;
+ vh = skb_vlan_eth_hdr(skb);
protocol = vh->h_vlan_encapsulated_proto;
flags = FLAGS_VLAN_TAGGED;

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
index 92930a055cbc..41894d154013 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
@@ -318,7 +318,7 @@ static void qlcnic_send_filter(struct qlcnic_adapter *adapter,

if (adapter->flags & QLCNIC_VLAN_FILTERING) {
if (protocol == ETH_P_8021Q) {
- vh = (struct vlan_ethhdr *)skb->data;
+ vh = skb_vlan_eth_hdr(skb);
vlan_id = ntohs(vh->h_vlan_TCI);
} else if (skb_vlan_tag_present(skb)) {
vlan_id = skb_vlan_tag_get(skb);
@@ -468,7 +468,7 @@ static int qlcnic_tx_pkt(struct qlcnic_adapter *adapter,
u32 producer = tx_ring->producer;

if (protocol == ETH_P_8021Q) {
- vh = (struct vlan_ethhdr *)skb->data;
+ vh = skb_vlan_eth_hdr(skb);
flags = QLCNIC_FLAGS_VLAN_TAGGED;
vlan_tci = ntohs(vh->h_vlan_TCI);
protocol = ntohs(vh->h_vlan_encapsulated_proto);
diff --git a/drivers/net/ethernet/sfc/tx_tso.c b/drivers/net/ethernet/sfc/tx_tso.c
index 898e5c61d908..d381d8164f07 100644
--- a/drivers/net/ethernet/sfc/tx_tso.c
+++ b/drivers/net/ethernet/sfc/tx_tso.c
@@ -147,7 +147,7 @@ static __be16 efx_tso_check_protocol(struct sk_buff *skb)
EFX_WARN_ON_ONCE_PARANOID(((struct ethhdr *)skb->data)->h_proto !=
protocol);
if (protocol == htons(ETH_P_8021Q)) {
- struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
+ struct vlan_ethhdr *veh = skb_vlan_eth_hdr(skb);

protocol = veh->h_vlan_encapsulated_proto;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8f543c3ab5c5..918de65fb707 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4554,13 +4554,10 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)

static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
{
- struct vlan_ethhdr *veth;
- __be16 vlan_proto;
+ struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
+ __be16 vlan_proto = veth->h_vlan_proto;
u16 vlanid;

- veth = (struct vlan_ethhdr *)skb->data;
- vlan_proto = veth->h_vlan_proto;
-
if ((vlan_proto == htons(ETH_P_8021Q) &&
dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
(vlan_proto == htons(ETH_P_8021AD) &&
diff --git a/drivers/staging/gdm724x/gdm_lte.c b/drivers/staging/gdm724x/gdm_lte.c
index 671ee8843c88..5703a9ddb6d0 100644
--- a/drivers/staging/gdm724x/gdm_lte.c
+++ b/drivers/staging/gdm724x/gdm_lte.c
@@ -349,7 +349,7 @@ static s32 gdm_lte_tx_nic_type(struct net_device *dev, struct sk_buff *skb)
/* Get ethernet protocol */
eth = (struct ethhdr *)skb->data;
if (ntohs(eth->h_proto) == ETH_P_8021Q) {
- vlan_eth = (struct vlan_ethhdr *)skb->data;
+ vlan_eth = skb_vlan_eth_hdr(skb);
mac_proto = ntohs(vlan_eth->h_vlan_encapsulated_proto);
network_data = skb->data + VLAN_ETH_HLEN;
nic_type |= NIC_TYPE_F_VLAN;
@@ -435,7 +435,7 @@ static netdev_tx_t gdm_lte_tx(struct sk_buff *skb, struct net_device *dev)
* driver based on the NIC mac
*/
if (nic_type & NIC_TYPE_F_VLAN) {
- struct vlan_ethhdr *vlan_eth = (struct vlan_ethhdr *)skb->data;
+ struct vlan_ethhdr *vlan_eth = skb_vlan_eth_hdr(skb);

nic->vlan_id = ntohs(vlan_eth->h_vlan_TCI) & VLAN_VID_MASK;
data_buf = skb->data + (VLAN_ETH_HLEN - ETH_HLEN);
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 90b76d63c11c..3698f2b391cd 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -62,6 +62,14 @@ static inline struct vlan_ethhdr *vlan_eth_hdr(const struct sk_buff *skb)
return (struct vlan_ethhdr *)skb_mac_header(skb);
}

+/* Prefer this version in TX path, instead of
+ * skb_reset_mac_header() + vlan_eth_hdr()
+ */
+static inline struct vlan_ethhdr *skb_vlan_eth_hdr(const struct sk_buff *skb)
+{
+ return (struct vlan_ethhdr *)skb->data;
+}
+
#define VLAN_PRIO_MASK 0xe000 /* Priority Code Point */
#define VLAN_PRIO_SHIFT 13
#define VLAN_CFI_MASK 0x1000 /* Canonical Format Indicator / Drop Eligible Indicator */
@@ -529,7 +537,7 @@ static inline void __vlan_hwaccel_put_tag(struct sk_buff *skb,
*/
static inline int __vlan_get_tag(const struct sk_buff *skb, u16 *vlan_tci)
{
- struct vlan_ethhdr *veth = (struct vlan_ethhdr *)skb->data;
+ struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);

if (!eth_type_vlan(veth->h_vlan_proto))
return -EINVAL;
@@ -713,7 +721,7 @@ static inline bool skb_vlan_tagged_multi(struct sk_buff *skb)
if (unlikely(!pskb_may_pull(skb, VLAN_ETH_HLEN)))
return false;

- veh = (struct vlan_ethhdr *)skb->data;
+ veh = skb_vlan_eth_hdr(skb);
protocol = veh->h_vlan_encapsulated_proto;
}

diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 125f4628687c..d3fdf82282af 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -439,7 +439,7 @@ void batadv_interface_rx(struct net_device *soft_iface,
if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
goto dropped;

- vhdr = (struct vlan_ethhdr *)skb->data;
+ vhdr = skb_vlan_eth_hdr(skb);

/* drop batman-in-batman packets to prevent loops */
if (vhdr->h_vlan_encapsulated_proto != htons(ETH_P_BATMAN))
--
2.34.1

2023-03-22 23:41:46

[permalink] [raw]

Subject: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX

ocelot_xmit_get_vlan_info() calls __skb_vlan_pop() as the most
appropriate helper I could find which strips away a VLAN header.
That's all I need it to do, but __skb_vlan_pop() has more logic, which
will become incompatible with the future revert of commit 6d1ccff62780
("net: reset mac header in dev_start_xmit()").

Namely, it performs a sanity check on skb_mac_header(), which will stop
being set after the above revert, so it will return an error instead of
removing the VLAN tag.

ocelot_xmit_get_vlan_info() gets called in 2 circumstances:

(1) the port is under a VLAN-aware bridge and the bridge sends
VLAN-tagged packets

(2) the port is under a VLAN-aware bridge and somebody else (an 8021q
upper) sends VLAN-tagged packets (using a VID that isn't in the
bridge vlan tables)

In case (1), there is actually no bug to defend against, because
br_dev_xmit() calls skb_reset_mac_header() and things continue to work.

However, in case (2), illustrated using the commands below, it can be
seen that our intervention is needed, since __skb_vlan_pop() complains:

$ ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up
$ ip link set $eth master br0 && ip link set $eth up
$ ip link add link $eth name $eth.100 type vlan id 100 && ip link set $eth.100 up
$ ip addr add 192.168.100.1/24 dev $eth.100
$ # needed to work around an apparent DSA RX filtering bug
$ ip link set $eth promisc on

I could fend off the checks in __skb_vlan_pop() with some
skb_mac_header_was_set() calls, but seeing how few callers of
__skb_vlan_pop() there are from TX paths, that seems rather
unproductive.

As an alternative solution, extract the bare minimum logic to strip a
VLAN header, and move it to a new helper named vlan_remove_tag(), close
to the definition of vlan_insert_tag(). Document it appropriately and
make ocelot_xmit_get_vlan_info() call this smaller helper instead.

Seeing that it doesn't appear illegal to test skb->protocol in the TX
path, I guess it would be a good for vlan_remove_tag() to also absorb
the vlan_set_encap_proto() function call.

Signed-off-by: Vladimir Oltean <[email protected]>
---
include/linux/if_vlan.h | 20 ++++++++++++++++++++
net/core/skbuff.c | 8 +-------
net/dsa/tag_ocelot.c | 2 +-
3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 3698f2b391cd..4d54814143a8 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -685,6 +685,26 @@ static inline void vlan_set_encap_proto(struct sk_buff *skb,
skb->protocol = htons(ETH_P_802_2);
}

+/**
+ * vlan_remove_tag - remove outer VLAN tag from payload
+ * @skb: skbuff to remove tag from
+ *
+ * Expects the skb to contain a VLAN tag in the payload, and to have skb->data
+ * pointing at the mac header.
+ *
+ * Returns a new pointer to skb->data, or NULL on failure to pull.
+ */
+static inline void *vlan_remove_tag(struct sk_buff *skb, u16 *vlan_tci)
+{
+ struct vlan_hdr *vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
+
+ *vlan_tci = ntohs(vhdr->h_vlan_TCI);
+
+ memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
+ vlan_set_encap_proto(skb, vhdr);
+ return __skb_pull(skb, VLAN_HLEN);
+}
+
/**
* skb_vlan_tagged - check if skb is vlan tagged.
* @skb: skbuff to query
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 050a875d09c5..0a7c058d4849 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5939,7 +5939,6 @@ EXPORT_SYMBOL(skb_ensure_writable);
*/
int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
{
- struct vlan_hdr *vhdr;
int offset = skb->data - skb_mac_header(skb);
int err;

@@ -5955,13 +5954,8 @@ int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)

skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);

- vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
- *vlan_tci = ntohs(vhdr->h_vlan_TCI);
-
- memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
- __skb_pull(skb, VLAN_HLEN);
+ vlan_remove_tag(skb, vlan_tci);

- vlan_set_encap_proto(skb, vhdr);
skb->mac_header += VLAN_HLEN;

if (skb_network_offset(skb) < ETH_HLEN)
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index 73ee09de1a3a..20bf7074d5a6 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -30,7 +30,7 @@ static void ocelot_xmit_get_vlan_info(struct sk_buff *skb, struct dsa_port *dp,
br_vlan_get_proto(br, &proto);

if (ntohs(hdr->h_vlan_proto) == proto) {
- __skb_vlan_pop(skb, &tci);
+ vlan_remove_tag(skb, &tci);
*vlan_tci = tci;
} else {
rcu_read_lock();
--
2.34.1

2023-03-22 23:42:24

[permalink] [raw]

Subject: [PATCH net-next 8/9] net: dsa: update TX path comments to not mention skb_mac_header()

Once commit 6d1ccff62780 ("net: reset mac header in dev_start_xmit()")
will be reverted, it will no longer be true that skb->data points at
skb_mac_header(skb) - since the skb->mac_header will not be set - so
stop saying that, and just say that it points to the MAC header.

I've reviewed vlan_insert_tag() and it does not *actually* depend on
skb_mac_header(), so reword that to avoid the confusion.

Signed-off-by: Vladimir Oltean <[email protected]>
---
net/dsa/tag.h | 2 +-
net/dsa/tag_8021q.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/dsa/tag.h b/net/dsa/tag.h
index 7cfbca824f1c..32d12f4a9d73 100644
--- a/net/dsa/tag.h
+++ b/net/dsa/tag.h
@@ -229,7 +229,7 @@ static inline void *dsa_etype_header_pos_rx(struct sk_buff *skb)
return skb->data - 2;
}

-/* On TX, skb->data points to skb_mac_header(skb), which means that EtherType
+/* On TX, skb->data points to the MAC header, which means that EtherType
* header taggers start exactly where the EtherType is (the EtherType is
* treated as part of the DSA header).
*/
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 5ee9ef00954e..cbdfc392f7e0 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -461,8 +461,8 @@ EXPORT_SYMBOL_GPL(dsa_tag_8021q_unregister);
struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
u16 tpid, u16 tci)
{
- /* skb->data points at skb_mac_header, which
- * is fine for vlan_insert_tag.
+ /* skb->data points at the MAC header, which is fine
+ * for vlan_insert_tag().
*/
return vlan_insert_tag(skb, htons(tpid), tci);
}
--
2.34.1

2023-03-22 23:42:50

[permalink] [raw]

Subject: [PATCH net-next 7/9] net: dsa: tag_sja1105: replace skb_mac_header() with vlan_eth_hdr()

This is a cosmetic patch which consolidates the code to use the helper
function offered by if_vlan.h.

Signed-off-by: Vladimir Oltean <[email protected]>
---
net/dsa/tag_sja1105.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index a7ca97b7ac9e..a5f3b73da417 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -516,7 +516,7 @@ static bool sja1110_skb_has_inband_control_extension(const struct sk_buff *skb)
static void sja1105_vlan_rcv(struct sk_buff *skb, int *source_port,
int *switch_id, int *vbid, u16 *vid)
{
- struct vlan_ethhdr *hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
+ struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
u16 vlan_tci;

if (skb_vlan_tag_present(skb))
--
2.34.1

2023-03-22 23:42:57

[permalink] [raw]

Subject: [PATCH net-next 6/9] net: dsa: tag_sja1105: don't rely on skb_mac_header() in TX paths

skb_mac_header() will no longer be available in the TX path when
reverting commit 6d1ccff62780 ("net: reset mac header in
dev_start_xmit()"). As preparation for that, let's use
skb_vlan_eth_hdr() to get to the VLAN header instead, which assumes it's
located at skb->data (assumption which holds true here).

Signed-off-by: Vladimir Oltean <[email protected]>
---
net/dsa/tag_sja1105.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 1c2ceba4771b..a7ca97b7ac9e 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -256,7 +256,7 @@ static struct sk_buff *sja1105_pvid_tag_control_pkt(struct dsa_port *dp,
return NULL;
}

- hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
+ hdr = skb_vlan_eth_hdr(skb);

/* If skb is already VLAN-tagged, leave that VLAN ID in place */
if (hdr->h_vlan_proto == xmit_tpid)
--
2.34.1

2023-03-23 16:10:08

[permalink] [raw]

Subject: Re: [PATCH net-next 2/9] net: vlan: introduce skb_vlan_eth_hdr()

On Thu, Mar 23, 2023 at 01:38:16AM +0200, Vladimir Oltean wrote:
> Similar to skb_eth_hdr() introduced in commit 96cc4b69581d ("macvlan: do
> not assume mac_header is set in macvlan_broadcast()"), let's introduce a
> skb_vlan_eth_hdr() helper which can be used in TX-only code paths to get
> to the VLAN header based on skb->data rather than based on the
> skb_mac_header(skb).
>
> We also consolidate the drivers that dereference skb->data to go through
> this helper.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-03-23 16:14:31

[permalink] [raw]

Subject: Re: [PATCH net-next 6/9] net: dsa: tag_sja1105: don't rely on skb_mac_header() in TX paths

On Thu, Mar 23, 2023 at 01:38:20AM +0200, Vladimir Oltean wrote:
> skb_mac_header() will no longer be available in the TX path when
> reverting commit 6d1ccff62780 ("net: reset mac header in
> dev_start_xmit()"). As preparation for that, let's use
> skb_vlan_eth_hdr() to get to the VLAN header instead, which assumes it's
> located at skb->data (assumption which holds true here).
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-03-23 16:23:20

[permalink] [raw]

Subject: Re: [PATCH net-next 7/9] net: dsa: tag_sja1105: replace skb_mac_header() with vlan_eth_hdr()

On Thu, Mar 23, 2023 at 01:38:21AM +0200, Vladimir Oltean wrote:
> This is a cosmetic patch which consolidates the code to use the helper
> function offered by if_vlan.h.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-03-23 16:30:58

[permalink] [raw]

Subject: Re: [PATCH net-next 8/9] net: dsa: update TX path comments to not mention skb_mac_header()

On Thu, Mar 23, 2023 at 01:38:22AM +0200, Vladimir Oltean wrote:
> Once commit 6d1ccff62780 ("net: reset mac header in dev_start_xmit()")
> will be reverted, it will no longer be true that skb->data points at
> skb_mac_header(skb) - since the skb->mac_header will not be set - so
> stop saying that, and just say that it points to the MAC header.
>
> I've reviewed vlan_insert_tag() and it does not *actually* depend on
> skb_mac_header(), so reword that to avoid the confusion.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-03-23 16:31:18

[permalink] [raw]

Subject: Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX

On Thu, Mar 23, 2023 at 01:38:23AM +0200, Vladimir Oltean wrote:
> ocelot_xmit_get_vlan_info() calls __skb_vlan_pop() as the most
> appropriate helper I could find which strips away a VLAN header.
> That's all I need it to do, but __skb_vlan_pop() has more logic, which
> will become incompatible with the future revert of commit 6d1ccff62780
> ("net: reset mac header in dev_start_xmit()").
>
> Namely, it performs a sanity check on skb_mac_header(), which will stop
> being set after the above revert, so it will return an error instead of
> removing the VLAN tag.
>
> ocelot_xmit_get_vlan_info() gets called in 2 circumstances:
>
> (1) the port is under a VLAN-aware bridge and the bridge sends
> VLAN-tagged packets
>
> (2) the port is under a VLAN-aware bridge and somebody else (an 8021q
> upper) sends VLAN-tagged packets (using a VID that isn't in the
> bridge vlan tables)
>
> In case (1), there is actually no bug to defend against, because
> br_dev_xmit() calls skb_reset_mac_header() and things continue to work.
>
> However, in case (2), illustrated using the commands below, it can be
> seen that our intervention is needed, since __skb_vlan_pop() complains:
>
> $ ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up
> $ ip link set $eth master br0 && ip link set $eth up
> $ ip link add link $eth name $eth.100 type vlan id 100 && ip link set $eth.100 up
> $ ip addr add 192.168.100.1/24 dev $eth.100
> $ # needed to work around an apparent DSA RX filtering bug
> $ ip link set $eth promisc on
>
> I could fend off the checks in __skb_vlan_pop() with some
> skb_mac_header_was_set() calls, but seeing how few callers of
> __skb_vlan_pop() there are from TX paths, that seems rather
> unproductive.
>
> As an alternative solution, extract the bare minimum logic to strip a
> VLAN header, and move it to a new helper named vlan_remove_tag(), close
> to the definition of vlan_insert_tag(). Document it appropriately and
> make ocelot_xmit_get_vlan_info() call this smaller helper instead.
>
> Seeing that it doesn't appear illegal to test skb->protocol in the TX
> path, I guess it would be a good for vlan_remove_tag() to also absorb
> the vlan_set_encap_proto() function call.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-03-23 16:33:32

by Eric Dumazet

[permalink] [raw]

Subject: Re: [PATCH net-next 0/9] Remove skb_mac_header() dependency in DSA xmit path

On Wed, Mar 22, 2023 at 4:38 PM Vladimir Oltean <[email protected]> wrote:
>
> Eric started working on removing skb_mac_header() assumptions from the
> networking xmit path, and I offered to help for DSA:
> https://lore.kernel.org/netdev/[email protected]/
>
> The majority of this patch set is a straightforward replacement of
> skb_mac_header() with skb->data (hidden either behind skb_eth_hdr(), or
> behind skb_vlan_eth_hdr()). The only patch which is more "interesting"
> is 9/9.
>

SGTM, thanks a lot !

For the whole series :

Reviewed-by: Eric Dumazet <[email protected]>

2023-03-23 16:36:08

[permalink] [raw]

Subject: Re: [PATCH net-next 2/9] net: vlan: introduce skb_vlan_eth_hdr()

On 3/22/23 16:38, Vladimir Oltean wrote:
> Similar to skb_eth_hdr() introduced in commit 96cc4b69581d ("macvlan: do
> not assume mac_header is set in macvlan_broadcast()"), let's introduce a
> skb_vlan_eth_hdr() helper which can be used in TX-only code paths to get
> to the VLAN header based on skb->data rather than based on the
> skb_mac_header(skb).
>
> We also consolidate the drivers that dereference skb->data to go through
> this helper.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-03-23 16:37:06

[permalink] [raw]

Subject: Re: [PATCH net-next 6/9] net: dsa: tag_sja1105: don't rely on skb_mac_header() in TX paths

On 3/22/23 16:38, Vladimir Oltean wrote:
> skb_mac_header() will no longer be available in the TX path when
> reverting commit 6d1ccff62780 ("net: reset mac header in
> dev_start_xmit()"). As preparation for that, let's use
> skb_vlan_eth_hdr() to get to the VLAN header instead, which assumes it's
> located at skb->data (assumption which holds true here).
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-03-23 16:37:11

[permalink] [raw]

Subject: Re: [PATCH net-next 7/9] net: dsa: tag_sja1105: replace skb_mac_header() with vlan_eth_hdr()

On 3/22/23 16:38, Vladimir Oltean wrote:
> This is a cosmetic patch which consolidates the code to use the helper
> function offered by if_vlan.h.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-03-23 16:37:46

[permalink] [raw]

Subject: Re: [PATCH net-next 8/9] net: dsa: update TX path comments to not mention skb_mac_header()

On 3/22/23 16:38, Vladimir Oltean wrote:
> Once commit 6d1ccff62780 ("net: reset mac header in dev_start_xmit()")
> will be reverted, it will no longer be true that skb->data points at
> skb_mac_header(skb) - since the skb->mac_header will not be set - so
> stop saying that, and just say that it points to the MAC header.
>
> I've reviewed vlan_insert_tag() and it does not *actually* depend on
> skb_mac_header(), so reword that to avoid the confusion.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>

}

--
Florian

2023-03-23 16:39:17

[permalink] [raw]

Subject: Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX

On 3/22/23 16:38, Vladimir Oltean wrote:
> ocelot_xmit_get_vlan_info() calls __skb_vlan_pop() as the most
> appropriate helper I could find which strips away a VLAN header.
> That's all I need it to do, but __skb_vlan_pop() has more logic, which
> will become incompatible with the future revert of commit 6d1ccff62780
> ("net: reset mac header in dev_start_xmit()").
>
> Namely, it performs a sanity check on skb_mac_header(), which will stop
> being set after the above revert, so it will return an error instead of
> removing the VLAN tag.
>
> ocelot_xmit_get_vlan_info() gets called in 2 circumstances:
>
> (1) the port is under a VLAN-aware bridge and the bridge sends
> VLAN-tagged packets
>
> (2) the port is under a VLAN-aware bridge and somebody else (an 8021q
> upper) sends VLAN-tagged packets (using a VID that isn't in the
> bridge vlan tables)
>
> In case (1), there is actually no bug to defend against, because
> br_dev_xmit() calls skb_reset_mac_header() and things continue to work.
>
> However, in case (2), illustrated using the commands below, it can be
> seen that our intervention is needed, since __skb_vlan_pop() complains:
>
> $ ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up
> $ ip link set $eth master br0 && ip link set $eth up
> $ ip link add link $eth name $eth.100 type vlan id 100 && ip link set $eth.100 up
> $ ip addr add 192.168.100.1/24 dev $eth.100
> $ # needed to work around an apparent DSA RX filtering bug
> $ ip link set $eth promisc on
>
> I could fend off the checks in __skb_vlan_pop() with some
> skb_mac_header_was_set() calls, but seeing how few callers of
> __skb_vlan_pop() there are from TX paths, that seems rather
> unproductive.
>
> As an alternative solution, extract the bare minimum logic to strip a
> VLAN header, and move it to a new helper named vlan_remove_tag(), close
> to the definition of vlan_insert_tag(). Document it appropriately and
> make ocelot_xmit_get_vlan_info() call this smaller helper instead.
>
> Seeing that it doesn't appear illegal to test skb->protocol in the TX
> path, I guess it would be a good for vlan_remove_tag() to also absorb
> the vlan_set_encap_proto() function call.
>
> Signed-off-by: Vladimir Oltean <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2023-03-23 16:39:54