Re: [PATCH net v4 1/2] flow_dissector: do not dissect PPPoE PFC frames

[Simon Horman <horms@kernel.org>]

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 <qingfang.deng@linux.dev>
> ---
> 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
>