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:   Thu, 23 Mar 2023 17:14:23 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     netdev@vger.kernel.org, "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
Subject: Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the
 relevant portion of __skb_vlan_pop() on TX
Message-ID: <ZBx63wbdoXnLlGMK@corigine.com>
References: <20230322233823.1806736-1-vladimir.oltean@nxp.com>
 <20230322233823.1806736-10-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230322233823.1806736-10-vladimir.oltean@nxp.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?qfyCizwuGD8hkEt4k+K4OYlxNyxyz9GXgJHtP4r3lDO127M7RgnJWoDDYPgE?=
 =?us-ascii?Q?E+tkajwpR7OdSA2ox780VytZMzzsWuOhLCavOo+U9DveeKYhSF4h/8Ro124M?=
 =?us-ascii?Q?sn2CX3Lz1T0H5wlv1VJfLkQqxDAL6zGUbkikLZoNODeW6Gk17pamaYcj+309?=
 =?us-ascii?Q?HkWImEQ9oLQRFTgRLKS1JFkhI2Uvb0SGyIBXmmHRInkK6TGTMVY7JK2sZmPk?=
 =?us-ascii?Q?nInP4sL1Ednnbr7vi/lHZoMjwLigjdNyRozpX8eeZ1xwevEwOmJINF68/qgr?=
 =?us-ascii?Q?XtulC16LIPas5d5S/0vQrhyn/ZKkXUC1/CYdHMWCKwLR9lkorWwaSeau1jUX?=
 =?us-ascii?Q?fFatwt3wOuCo2z3NTeNDnbLCXUX+czDevGvhv6F7MioP6co1hAlQWlCVZRDD?=
 =?us-ascii?Q?bTU5sfLb5wAZfJIS9oWl3VuGFxqbc5CtH9falb/uYV7ZFWZaPTqr02q4DA2C?=
 =?us-ascii?Q?l4l3nFZvp5I+tfU124Wa2vCXey42LrdRZ+pYkNKdppOUPtD/XEOfvMUEuOte?=
 =?us-ascii?Q?sf2kiAMwOdMyLOQpVWpEqe9ZwqfgzX2Wl0oeFFQMriFTouhP4HrGg0PYkFps?=
 =?us-ascii?Q?LhE97jaBCL74yxI++LGE4R13CB20Q+W9hvZn7Db7lW/U3nPnyIlnW2lTN8RH?=
 =?us-ascii?Q?+g75AfjglTgYzxlRgB+Xvj5Xws6lWos16iXBBvEeBeF+ePWDqK22XPH5C40v?=
 =?us-ascii?Q?CZhuDtci+0z3VXLCJN+0+eoptuIpSiX0/QvPm5npuENUCOJ5HUNKNbyYXAwP?=
 =?us-ascii?Q?9PLbmiMZQmq7o1MPDN2QBb7aywXO7xnedhjG7CSn385dPsYU8sCfncghWVeY?=
 =?us-ascii?Q?pByNb+8RSW8ICmg/dtqBsjZTUnK625Lgvv8xrQquATK2+DKQ3cir+0dCK9TQ?=
 =?us-ascii?Q?w/ctVkyyKSY9mgSawn2o8aa7ZbyJ7FCfxVY39S6uvoZL7Dcv0iCJSsRehpna?=
 =?us-ascii?Q?NcsKoHZT4ePEgpvcsJ4oNS7H1/Ecew1I5jhyDBq7+ptEpYuz+FEDPXGCSKWX?=
 =?us-ascii?Q?ccN7j8A7tl0iwvGEtasBykBN0zxdG7dDkRZDKii/QWugXSKBqqpHLhlWVR/U?=
 =?us-ascii?Q?jESFW1RZMIr7HdugrmK8ysRDs4Ikneaf9bu09sRiGZBfl8Qvh61y1uENlnxe?=
 =?us-ascii?Q?DKLRYhmznldO+qjRUgyZV1oxQBScVP818TVmPKaBAAo70bV/7MgkVV/tEMRG?=
 =?us-ascii?Q?XyUkfXvvIvbQWQ+9VVssI3FkEalmul6hWUuezKYz93PDeLapm5+34Ai/eF0E?=
 =?us-ascii?Q?04n8DzI4kaquxo8mQzcs4uXztWk02DJuZWjuIgXdWAKFzipLrZ6pAnS3jzDC?=
 =?us-ascii?Q?COUoqdF+4on+Mc+1eZ9i0493PHZ9GlWdd7jrNNx0rwjayNmEy/oj9Lo4zsUM?=
 =?us-ascii?Q?V6dlRAjmQxxY3sYlw7XPY1EOGDf6RrNVQgfzg/UPViWJqTs/rG0VV209bEuW?=
 =?us-ascii?Q?uDqWMeZbR7YBQlhIXtm+C2m1O/0962ahihl+sinI7/LF/hZxDpOb/bXbI4b0?=
 =?us-ascii?Q?BQWWbwZl8HLo095oscXApRwGW3OM0OuGKOHFyA9rAES9/yVEdsJmQIttwR2P?=
 =?us-ascii?Q?cjiS62AimOz9BGdPC6PkmsZod24HkiRdzo+ha6zp895mZHE0tuTf309/PSwl?=
 =?us-ascii?Q?eTX3XDcOuJa8nvUuzGTJEICcOHn9S0933F9HfgrMXm4+ntsc6eEjAKGOEi5l?=
 =?us-ascii?Q?MGM+Mg=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 93d1d4ad-48f3-4a96-fa8e-08db2bb9add5
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Mar 2023 16:14:29.0670
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: v7AzgxMyxC3mjfrSwsTBPI411FqFFFOLz1/iPBV1aUns24qPYNqbrulyJSjz75Vy7f/q4RS6inlirGn8mfBcSYWgv2RjQgbV8lgIA4lXkSE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR13MB6189
X-Spam-Status: No, score=-0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=unavailable 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

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 <vladimir.oltean@nxp.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>