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;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     netdev@vger.kernel.org
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        linux-kernel@vger.kernel.org,
        Simon Horman <simon.horman@corigine.com>
Subject: [PATCH v2 net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan_pop() on TX
Date:   Fri, 21 Apr 2023 01:56:01 +0300
Message-Id: <20230420225601.2358327-10-vladimir.oltean@nxp.com>
In-Reply-To: <20230420225601.2358327-1-vladimir.oltean@nxp.com>
References: <20230420225601.2358327-1-vladimir.oltean@nxp.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0s4dBz43+w+hVcPizI+mtqftpe3fvThvs8Y8zUwKN+a1hm7qbsK6OOliQi4F?=
 =?us-ascii?Q?jhm3JQNPt9n8z7Zc2G7dzuHaf5lW2IizWHhzNxBVeS6Z40vGjArNkAg2+XiP?=
 =?us-ascii?Q?uPKAejK22fg+a1hmZSHMJQOoeR0Gr9MPGB00tl/kBcBVehcynVyG+0ByzXAL?=
 =?us-ascii?Q?+LuYmdkqB8i295dBRVauJYn9DqKj0mxzs8t+2LW8S0YrNHsZw3HRfAl441iq?=
 =?us-ascii?Q?bD9KgXlCFT283+5vJIEDY/90bJbWWIAjMI8ooeRAnAyWN4WXITMtUU5iBjfD?=
 =?us-ascii?Q?gf58SoYDZJp1qSQS3TTwbw0I9qbEZ/wLrBNnPDVouCybmBgHEtKxLrtjrXd6?=
 =?us-ascii?Q?9Z63QH/D/9tESn5/eeEmNH0U8B6l0nLQUrAPBkvQiLRlxpydsXXUYWcFVXZ/?=
 =?us-ascii?Q?j4L+579ngBukbZ1Fp/njWs/qS8VuqRW5A4gx5+LYCLi6PbFCStlx/UvY3oB8?=
 =?us-ascii?Q?tCz7bJD18SH2zhf1QxG4RGzvgslOGBQaBQAAtLBn0tcWIeYHUi+zq0MVTgK9?=
 =?us-ascii?Q?efn6pSXVH8s0DYCTVP/cnQdRzkPit97vp2HW7jZ0+PTxq0nAcUf/mNDVgDFy?=
 =?us-ascii?Q?hxLw9023oveuF1qIb8RmOIA42q+HsGwLnK1FfP0AUU3B6mDn9/Jhp7RtAvtg?=
 =?us-ascii?Q?wFiE5Ucq9Lxg0gpXW1B9R4CsB08fNQYxXWxYk0KTT3ePCBjF2x1wdKxjue0m?=
 =?us-ascii?Q?yWsU/nxW92ds0y13uL6Nt+LIAXZBC1dlgQxXMnC6D0RuEMV+4MUfXKGcMO9e?=
 =?us-ascii?Q?IzOMtvgOmrxmdbqfNXQGrWXLLrkrR8sD0FN2WYBccFgTUIFQX9RCjM3qkXF3?=
 =?us-ascii?Q?lKsCtpGlEpgvwcG2FzDidIicKw/RNPHmXU9AWzRZ+rJKAczHCXrWKWrOgW/M?=
 =?us-ascii?Q?JgCxRmSf6C5UFoCQebl8vS1LHqKW0e8QJzz1fucIQPrqlDRJjyR+hrMxLHKq?=
 =?us-ascii?Q?hX9n7YLnaXLSguh5Jgv+f3zJillsCufZReu3i/l0va04CP4194ZQKS2qcziR?=
 =?us-ascii?Q?GA0qMSyW46QJzRVpsfPGFmtyM0k1ioYULhlbdR4++FNlB9LIfCTGrhsU7faj?=
 =?us-ascii?Q?Ils/IsGHIdesMeB0h30BeWZNH2hVi0RSGb5zp5EHHpCHx/PXTLDLlkFZ1BcP?=
 =?us-ascii?Q?hh1G7981uHjt9aSaAotT/MztpVFP2aBafGcIL14xB+wv0ZqqxJoexxs/2bPJ?=
 =?us-ascii?Q?slNUQCanGzJyECeel34szeq1mfX1TSTLYqxMBZ4kRd3Oj9vlVg0fZyjdg6iV?=
 =?us-ascii?Q?NsJpLnkpQt+yr6zUsDrxcOU4p5oJs0sFmj4gHpGuKyEIGayJM+6Io65Xt+d5?=
 =?us-ascii?Q?gugUjuBGpqkrgCAjmB5DLqgl6mLm6XmIAlxxG42qDNoiVpSw+NiEBKzCcwB9?=
 =?us-ascii?Q?1SvuuZmg/4Ak7e7jtIeTboNtFnJ/35TiNnK8UJguyHHZEXXsUIGDvV8wXbqz?=
 =?us-ascii?Q?G953fGiWoi0hLDCV2vUCSu2sjkBfLUKgb6SQlE8Cc7Oa3Zw1xN2wtO70Dj8y?=
 =?us-ascii?Q?QtzzxrgHO3M/aVEukWBxVbtcCzTw98Y9OlwICKLa2CKyDHaUMGmR6pKqARBe?=
 =?us-ascii?Q?HrL8TZV2tBjS9KRyLTxLweDpcBJ4zTSEbxdWPkUP2BVAVhkaEt/OstwQS1V+?=
 =?us-ascii?Q?EQ=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f34c1c0f-2259-4237-7218-08db41f27404
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2023 22:56:18.9062
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0LaxCqmuqKDqeL4beTUtoKQ9xqCu9DcDNigRvbjBXkZXi+Vi+q+XgC9lZy+w/LKrj3RXsUxmyGwaR/SDu4yezg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB9497
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,SPF_HELO_PASS,
        T_SCC_BODY_TEXT_LINE,T_SPF_PERMERROR autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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

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 <vladimir.oltean@nxp.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/if_vlan.h | 21 +++++++++++++++++++++
 net/core/skbuff.c       |  8 +-------
 net/dsa/tag_ocelot.c    |  2 +-
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 3698f2b391cd..0f40f379d75c 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -685,6 +685,27 @@ 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
+ * @vlan_tci: buffer to store value
+ *
+ * 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 768f9d04911f..3fbf32897b8d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5943,7 +5943,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;
 
@@ -5959,13 +5958,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