From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9FA063DFC7F; Fri, 10 Apr 2026 17:11:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775841061; cv=none; b=RTpzmUaIm2Q1LFXT4lTYyhHNVYUaEN8yVKonKeod6Mz/Wjn2o/ZrhLC743L733hNAjj2vG6jNrTCwz7a4CZ0KZcOKwjYwsCZEUSAYA4SUs2WBgeq81hM96p+1oBplbfRE+qijWdqIMkZi/b5ebla/SubBeEHanGUHX/PClDym10= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775841061; c=relaxed/simple; bh=8EN1zrzRDb6TUWeQhnImcMuuCiLJgGFlrg+fJ3ieqeM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=AkgkmV6NYWGS3DWrOhW4eaZaYvevdxZTtFzVWWaNGV7Fvv9UIAeliLSrFp0JlXdBRl0qDPTEK5zE4mQA/r4Oiv1TUSNiJ1v9XYQE8vI2M8vv9+7lJhv+chC9iwUnDZ8ixWEu9y7DXIjoqblX/cnmo1GLOAQTkM6VfmNr9WMNMJM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Om35ukAY; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Om35ukAY" Received: by smtp.kernel.org (Postfix) with ESMTPSA id AC6DBC19421; Fri, 10 Apr 2026 17:10:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1775841061; bh=8EN1zrzRDb6TUWeQhnImcMuuCiLJgGFlrg+fJ3ieqeM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Om35ukAYCsu3lL+iGdWj43PurDMUiw0WFo3TqoukIje7tE/iTOGM6ah3VwF1JVsH7 blKYhLGROX5nXAIerl7dW3ecfJEV9qevcicwl4oPEHxP0mdgLIXNyEIOARZCZ+uLti 53UF3ayiANI4WI8XZD4q7uGNKaJCUL61O6/0UbojfTnGc2nPAvxdSfmtwpfitINNT2 qIrBr6ZMl+PUXQbUCXW9gcDcSJGH0EE/metk1f30K7Z+HSKN5Q2zYL4zgbjOWJQ1FE hMwWpHE3OhfSYyG/BzytGJG0mg+1bP5ifA2W0rrU2tW4P2Zfim4VW9GCeLe1RNHBym KQx5vKVJ8g6UQ== Date: Fri, 10 Apr 2026 18:10:56 +0100 From: Simon Horman To: Qingfang Deng Cc: linux-ppp@vger.kernel.org, "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Guillaume Nault , Wojciech Drewek , Tony Nguyen , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Paul Mackerras , Jaco Kroon , James Carlson , Marcin Szycik Subject: Re: [PATCH net v4 1/2] flow_dissector: do not dissect PPPoE PFC frames Message-ID: <20260410171056.GD469338@kernel.org> References: <20260410033627.93786-1-qingfang.deng@linux.dev> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260410033627.93786-1-qingfang.deng@linux.dev> On Fri, Apr 10, 2026 at 11:36:20AM +0800, Qingfang Deng wrote: > RFC 2516 Section 7 states that Protocol Field Compression (PFC) is NOT > RECOMMENDED for PPPoE. In practice, pppd does not support negotiating > PFC for PPPoE sessions, and the flow dissector driver has assumed an > uncompressed frame until the blamed commit. > > During the review process of that commit [1], support for PFC is > suggested. However, having a compressed (1-byte) protocol field means > the subsequent PPP payload is shifted by one byte, causing 4-byte > misalignment for the network header and an unaligned access exception > on some architectures. > > The exception can be reproduced by sending a PPPoE PFC frame to an > ethernet interface of a MIPS board, with RPS enabled, even if no PPPoE > session is active on that interface: > > $ 0 : 00000000 80c40000 00000000 85144817 > $ 4 : 00000008 00000100 80a75758 81dc9bb8 > $ 8 : 00000010 8087ae2c 0000003d 00000000 > $12 : 000000e0 00000039 00000000 00000000 > $16 : 85043240 80a75758 81dc9bb8 00006488 > $20 : 0000002f 00000007 85144810 80a70000 > $24 : 81d1bda0 00000000 > $28 : 81dc8000 81dc9aa8 00000000 805ead08 > Hi : 00009d51 > Lo : 2163358a > epc : 805e91f0 __skb_flow_dissect+0x1b0/0x1b50 > ra : 805ead08 __skb_get_hash_net+0x74/0x12c > Status: 11000403 KERNEL EXL IE > Cause : 40800010 (ExcCode 04) > BadVA : 85144817 > PrId : 0001992f (MIPS 1004Kc) > Call Trace: > [<805e91f0>] __skb_flow_dissect+0x1b0/0x1b50 > [<805ead08>] __skb_get_hash_net+0x74/0x12c > [<805ef330>] get_rps_cpu+0x1b8/0x3fc > [<805fca70>] netif_receive_skb_list_internal+0x324/0x364 > [<805fd120>] napi_complete_done+0x68/0x2a4 > [<8058de5c>] mtk_napi_rx+0x228/0xfec > [<805fd398>] __napi_poll+0x3c/0x1c4 > [<805fd754>] napi_threaded_poll_loop+0x234/0x29c > [<805fd848>] napi_threaded_poll+0x8c/0xb0 > [<80053544>] kthread+0x104/0x12c > [<80002bd8>] ret_from_kernel_thread+0x14/0x1c > > Code: 02d51821 1060045b 00000000 <8c640000> 3084000f 2c820005 144001a2 00042080 8e220000 > > To reduce the attack surface and maintain performance, do not process > PPPoE PFC frames. While at it, avoid byte-swapping at runtime, restoring > the original behavior. > > [1] https://patch.msgid.link/20220630231016.GA392@debian.home > Fixes: 46126db9c861 ("flow_dissector: Add PPPoE dissectors") > Signed-off-by: Qingfang Deng > --- > Changes in v4: no new changes > Link to v3: https://lore.kernel.org/r/20260409031107.616630-1-qingfang.deng@linux.dev > Changes in v3: > Make ppp_proto_is_valid() private and fix kdoc warning, avoiding > gotchas if some out-of-tree modules use this function. > Link to v1: https://lore.kernel.org/r/20260407045743.174446-1-qingfang.deng@linux.dev > > include/linux/ppp_defs.h | 13 ------------- > net/core/flow_dissector.c | 39 +++++++++++++++++++++++---------------- > 2 files changed, 23 insertions(+), 29 deletions(-) > > diff --git a/include/linux/ppp_defs.h b/include/linux/ppp_defs.h > index b7e57fdbd413..45c0947fa404 100644 > --- a/include/linux/ppp_defs.h > +++ b/include/linux/ppp_defs.h > @@ -12,17 +12,4 @@ > > #define PPP_FCS(fcs, c) crc_ccitt_byte(fcs, c) > > -/** > - * ppp_proto_is_valid - checks if PPP protocol is valid > - * @proto: PPP protocol > - * > - * Assumes proto is not compressed. > - * Protocol is valid if the value is odd and the least significant bit of the > - * most significant octet is 0 (see RFC 1661, section 2). > - */ > -static inline bool ppp_proto_is_valid(u16 proto) > -{ > - return !!((proto & 0x0101) == 0x0001); > -} > - > #endif /* _PPP_DEFS_H_ */ > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 1b61bb25ba0e..64b843800370 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -1035,6 +1035,21 @@ static bool is_pppoe_ses_hdr_valid(const struct pppoe_hdr *hdr) > return hdr->ver == 1 && hdr->type == 1 && hdr->code == 0; > } > > +/** > + * ppp_proto_is_valid - checks if PPP protocol is valid > + * @proto: PPP protocol > + * > + * Assumes proto is not compressed. > + * Protocol is valid if the value is odd and the least significant bit of the > + * most significant octet is 0 (see RFC 1661, section 2). > + * > + * Return: Whether the PPP protocol is valid. > + */ > +static bool ppp_proto_is_valid(__be16 proto) > +{ > + return (proto & htons(0x0101)) == htons(0x0001); > +} > + > /** > * __skb_flow_dissect - extract the flow_keys struct and return it > * @net: associated network namespace, derived from @skb if NULL > @@ -1361,7 +1376,7 @@ bool __skb_flow_dissect(const struct net *net, > struct pppoe_hdr hdr; > __be16 proto; > } *hdr, _hdr; > - u16 ppp_proto; > + __be16 ppp_proto; I'm unclear of the relationship between changing the type of ppp_proto and the problem described in the patch description. And it is creating a log of churn in this patch. I suggest dropping it. > > hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr); > if (!hdr) { > @@ -1374,27 +1389,19 @@ bool __skb_flow_dissect(const struct net *net, > break; > } > > - /* least significant bit of the most significant octet > - * indicates if protocol field was compressed > - */ > - ppp_proto = ntohs(hdr->proto); > - if (ppp_proto & 0x0100) { > - ppp_proto = ppp_proto >> 8; > - nhoff += PPPOE_SES_HLEN - 1; > - } else { > - nhoff += PPPOE_SES_HLEN; > - } Could we go for something like this? ppp_proto = ntohs(hdr->proto); nhoff += PPPOE_SES_HLEN; /* Explanation of what is going on */ if (ppp_proto & 0x0100) ppp_proto = some invalid value like 0 > + ppp_proto = hdr->proto; > + nhoff += PPPOE_SES_HLEN; > > - if (ppp_proto == PPP_IP) { > + if (ppp_proto == htons(PPP_IP)) { > proto = htons(ETH_P_IP); > fdret = FLOW_DISSECT_RET_PROTO_AGAIN; > - } else if (ppp_proto == PPP_IPV6) { > + } else if (ppp_proto == htons(PPP_IPV6)) { > proto = htons(ETH_P_IPV6); > fdret = FLOW_DISSECT_RET_PROTO_AGAIN; > - } else if (ppp_proto == PPP_MPLS_UC) { > + } else if (ppp_proto == htons(PPP_MPLS_UC)) { > proto = htons(ETH_P_MPLS_UC); > fdret = FLOW_DISSECT_RET_PROTO_AGAIN; > - } else if (ppp_proto == PPP_MPLS_MC) { > + } else if (ppp_proto == htons(PPP_MPLS_MC)) { > proto = htons(ETH_P_MPLS_MC); > fdret = FLOW_DISSECT_RET_PROTO_AGAIN; > } else if (ppp_proto_is_valid(ppp_proto)) { > @@ -1412,7 +1419,7 @@ bool __skb_flow_dissect(const struct net *net, > FLOW_DISSECTOR_KEY_PPPOE, > target_container); > key_pppoe->session_id = hdr->hdr.sid; > - key_pppoe->ppp_proto = htons(ppp_proto); > + key_pppoe->ppp_proto = ppp_proto; > key_pppoe->type = htons(ETH_P_PPP_SES); > } > break; > -- > 2.43.0 >