[PATCH net] ppp: remove SC_MUST_COMP

[PATCH net] ppp: remove SC_MUST_COMP

[Qingfang Deng]

SC_MUST_COMP accidentally shares the same value with SC_DC_ERROR.
PPPIOCGFLAGS combines configuration flags with transmit and receive
state, so a userspace cannot distinguish them, and a read-modify-write
sequence can promote a decompression error into SC_MUST_COMP.

The flag entered Linux in 2005 with commit b3f9b92a6ec1 ("[PPP]: add PPP
MPPE encryption module"). The original patch submission [1] said pppd
would set it when CCP came up with MPPE enabled. That intended
integration [2] appears never to have landed upstream.

Remove the broken and unused SC_MUST_COMP from the UAPI, SC_MASK and
SC_FLAG_BITS, and remove its transmit and receive drop checks.
PPPIOCSFLAGS will discard this bit while PPPIOCGFLAGS can still report
it as SC_DC_ERROR.

Fixes: b3f9b92a6ec1 ("[PPP]: add PPP MPPE encryption module")
[1] https://lore.kernel.org/netdev/20050814141046.GB20944@lists.us.dell.com/
[2] https://lore.kernel.org/netdev/20050814190529.GA26802@lists.us.dell.com/
Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
---
Found and reproduced with LLM. Combined with the broken BSD-Compress on
64-bit, it results in a broken link:

1. First BSD failure: the kernel sets both SC_DC_FERROR and SC_DC_ERROR,
   then passes the failed packet to userspace. pppd closes CCP. Traffic
   falls back to uncompressed, but SC_MUST_COMP becomes accidentally
   latched.

2. Re-enable compression: send SIGUSR2 to pppd, to let BSD compression
   renegotiate. This is a documented pppd operation (man 8 pppd). The
   stale flag survives.

3. Second BSD failure: the kernel sets both error flags again. Now the
   receive check fires:

	if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
	    goto err;

   The kernel drops the failed packet before delivering it to pppd.
   Consequently, pppd does not perform its normal CCP shutdown and
   cleanup.

4. Subsequent incoming IP and CCP packets hit the same drop check,
   preventing normal recovery.
---
 drivers/net/ppp/ppp_generic.c  | 15 ++-------------
 include/uapi/linux/ppp-ioctl.h |  3 +--
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 1a610a18893b..84ca4e93ad9a 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -157,14 +157,13 @@ struct ppp {
 
 /*
  * Bits in flags: SC_NO_TCP_CCID, SC_CCP_OPEN, SC_CCP_UP, SC_LOOP_TRAFFIC,
- * SC_MULTILINK, SC_MP_SHORTSEQ, SC_MP_XSHORTSEQ, SC_COMP_TCP, SC_REJ_COMP_TCP,
- * SC_MUST_COMP
+ * SC_MULTILINK, SC_MP_SHORTSEQ, SC_MP_XSHORTSEQ, SC_COMP_TCP, SC_REJ_COMP_TCP.
  * Bits in rstate: SC_DECOMP_RUN, SC_DC_ERROR, SC_DC_FERROR.
  * Bits in xstate: SC_COMP_RUN
  */
 #define SC_FLAG_BITS	(SC_NO_TCP_CCID|SC_CCP_OPEN|SC_CCP_UP|SC_LOOP_TRAFFIC \
 			 |SC_MULTILINK|SC_MP_SHORTSEQ|SC_MP_XSHORTSEQ \
-			 |SC_COMP_TCP|SC_REJ_COMP_TCP|SC_MUST_COMP)
+			 |SC_COMP_TCP|SC_REJ_COMP_TCP)
 
 /*
  * Private data structure for each channel.
@@ -1862,13 +1861,6 @@ ppp_prepare_tx_skb(struct ppp *ppp, struct sk_buff **pskb)
 	/* try to do packet compression */
 	if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state &&
 	    proto != PPP_LCP && proto != PPP_CCP) {
-		if (!(ppp->flags & SC_CCP_UP) && (ppp->flags & SC_MUST_COMP)) {
-			if (net_ratelimit())
-				netdev_err(dev,
-					   "ppp: compression required but "
-					   "down - pkt dropped.\n");
-			goto drop;
-		}
 		new_skb = pad_compress_skb(ppp, skb);
 		if (!new_skb)
 			goto drop;
@@ -2416,9 +2408,6 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 	    (ppp->rstate & (SC_DC_FERROR | SC_DC_ERROR)) == 0)
 		skb = ppp_decompress_frame(ppp, skb);
 
-	if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
-		goto err;
-
 	/* At this point the "Protocol" field MUST be decompressed, either in
 	 * ppp_input(), ppp_decompress_frame() or in ppp_receive_mp_frame().
 	 */
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index 1cc5ce0ae062..672f7ac11a0b 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -39,8 +39,7 @@
 #define SC_LOG_RAWIN	0x00080000	/* log all chars received */
 #define SC_LOG_FLUSH	0x00100000	/* log all chars flushed */
 #define	SC_SYNC		0x00200000	/* synchronous serial mode */
-#define	SC_MUST_COMP    0x00400000	/* no uncompressed packets may be sent or received */
-#define	SC_MASK		0x0f600fff	/* bits that user can change */
+#define	SC_MASK		0x0f200fff	/* bits that user can change */
 
 /* state bits */
 #define SC_XMIT_BUSY	0x10000000	/* (used by isdn_ppp?) */
-- 
2.43.0

Re: [PATCH net] ppp: remove SC_MUST_COMP

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch deletes the exported `#define SC_MUST_COMP
  0x00400000` from include/uapi/linux/ppp-ioctl.h.

Pre-existing issues:
- [Medium] With SC_COMP_RUN set and SC_CCP_UP clear, a successfully
  compressed DEFLATE/BSD packet (len > 0) falls into the error branch of…

--

ppp: remove SC_MUST_COMP

This patch removes SC_MUST_COMP from the UAPI header, SC_MASK and
SC_FLAG_BITS. SC_MUST_COMP has the same value as SC_DC_ERROR, so a
read-modify-write through PPPIOCGFLAGS/PPPIOCSFLAGS can latch it by
accident. The transmit and receive drop checks for the flag in
ppp_generic.c are removed as well.

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 29847dc4bb454..f7b59060450fe 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c

[ ... ]

> @@ -1863,13 +1862,6 @@ ppp_prepare_tx_skb(struct ppp *ppp, struct sk_buff **pskb)
>  	/* try to do packet compression */
>  	if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state &&
>  	    proto != PPP_LCP && proto != PPP_CCP) {
> -		if (!(ppp->flags & SC_CCP_UP) && (ppp->flags & SC_MUST_COMP)) {
> -			if (net_ratelimit())
> -				netdev_err(dev,
> -					   "ppp: compression required but "
> -					   "down - pkt dropped.\n");
> -			goto drop;
> -		}
>  		new_skb = pad_compress_skb(ppp, skb);
>  		if (!new_skb)
>  			goto drop;

[Severity: Medium]
This is a pre-existing issue and wasn't introduced by this patch. While
looking at this path: can pad_compress_skb() drop packets that were
compressed successfully?

drivers/net/ppp/ppp_generic.c:pad_compress_skb() {
    ...
	if (len > 0 && (ppp->flags & SC_CCP_UP)) {
		...
	} else if (len == 0) {
		...
	} else {
		...
		consume_skb(new_skb);
		new_skb = NULL;
	}
    ...
}

Say SC_COMP_RUN is set but SC_CCP_UP is still clear. A DEFLATE or BSD
compressor that returns len > 0 then lands in the else branch. The
packet is dropped just as if the compressor had rejected it.

That state looks reachable. ppp_ccp_peek() handles an outbound
CCP_CONFACK only while SC_CCP_OPEN is set and SC_CCP_UP is clear, and
that is where it sets SC_COMP_RUN:

	case CCP_CONFACK:
		if ((ppp->flags & (SC_CCP_OPEN | SC_CCP_UP)) != SC_CCP_OPEN)
			break;
		...
			if (ppp->xcomp->comp_init(ppp->xc_state, dp, len,
						  ppp->file.index, 0,
						  READ_ONCE(ppp->debug)))
				ppp->xstate |= SC_COMP_RUN;

pppd sets SC_CCP_UP only once CCP reaches Opened.

When the drop happens, z_compress() has already updated its history and
advanced its sequence number:

	put_unaligned_be16(state->seqno, wptr);
	...
	++state->seqno;

After SC_CCP_UP is set, would the next compressed frame carry a sequence
number ahead of what the peer's z_decompress() expects?

	if (seq != (state->seqno & 0xffff)) {
		...
		return DECOMP_ERROR;
	}

If so, data sent in that window is lost, and recovery needs a CCP
Reset-Request/Reset-Ack cycle. The else branch dates from b3f9b92a6ec1,
and this patch leaves it unchanged.

[ ... ]

> diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
> index 1cc5ce0ae062e..672f7ac11a0b8 100644
> --- a/include/uapi/linux/ppp-ioctl.h
> +++ b/include/uapi/linux/ppp-ioctl.h
> @@ -39,8 +39,7 @@
>  #define SC_LOG_RAWIN	0x00080000	/* log all chars received */
>  #define SC_LOG_FLUSH	0x00100000	/* log all chars flushed */
>  #define	SC_SYNC		0x00200000	/* synchronous serial mode */
> -#define	SC_MUST_COMP    0x00400000	/* no uncompressed packets may be sent or received */

[Severity: Medium]
Does the fix need the define to be removed from the UAPI header?

PPPIOCSFLAGS already masks the user value with the kernel-internal
SC_FLAG_BITS:

drivers/net/ppp/ppp_generic.c:ppp_ioctl() {
    ...
		ppp->flags = val & SC_FLAG_BITS;
    ...
}

Removing the bit from SC_FLAG_BITS (and SC_MASK), together with the two
checks, looks like enough to stop SC_DC_ERROR from being stored as a
config flag.

Deleting the define is a separate change at the source level. Userspace
that includes <linux/ppp-ioctl.h>, directly or through <linux/if_ppp.h>,
and refers to SC_MUST_COMP would no longer build.

The commit message calls the flag unused, but the evidence is that one
pppd integration from 2005 never landed upstream. That does not rule out
out-of-tree pppd MPPE forks or other daemons that use the name.

With the Fixes: tag pointing at a 2005 commit, this header change will
likely be backported to the stable kernel-headers as well.

Would it work to keep the define, with a comment that the kernel ignores
it and that it aliases SC_DC_ERROR, while still dropping it from
SC_FLAG_BITS and SC_MASK and removing the checks?

> -#define	SC_MASK		0x0f600fff	/* bits that user can change */
> +#define	SC_MASK		0x0f200fff	/* bits that user can change */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029074935.40509-1-qingfang.deng%40linux.dev