Re: [PATCH net] ppp: remove SC_MUST_COMP

[netdev-bot+sashiko@kernel.org]

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