[PATCH] net/sched: reset sch_plug epoch state

[PATCH] net/sched: reset sch_plug epoch state

[shaos]

Hello,

sch_plug's reset callback purges the skb queue but leaves the private
packet-epoch counters intact.  If a buffered epoch is reset by a link
reset, a later TCQ_PLUG_RELEASE_ONE can use the old pkts_last_epoch
count as release credit and transmit a fresh packet that was never part
of the completed epoch.

The reproducer confirms this after link down/up: four buffered frames
are dropped by reset, then one new frame is held until a single release
command delivers it.  Three close controls do not deliver the frame.
The same fixed-kernel test keeps it queued.  This breaks the output
commit guarantee documented for checkpoint/replication users of the
plug qdisc.  It requires CAP_NET_ADMIN to configure/reset the qdisc and
is a correctness issue; no memory corruption or privilege escalation
was observed.

HEAD commit:    c03c1a14507dfaf8b856c2608f9ac216021fc222
Git tree:       30fd2ef82912c26cf3a58ad0ba6afc375fa851ac
Source version: Linux 7.3
Compiler:       gcc 11.4.0, GNU ld 2.38

The console logs, full kernel config, PoC, and initramfs runner are
available from one fixed-revision Gist:

Console output:
https://gist.githubusercontent.com/shaos26/f514dff98be5c90da07bc3467214ef10/raw/e16e1fa984b297ef7bf18e2c8a5992fc63daa062/console-output.txt
Kernel config:
https://gist.githubusercontent.com/shaos26/f514dff98be5c90da07bc3467214ef10/raw/e16e1fa984b297ef7bf18e2c8a5992fc63daa062/kernel-config.txt
PoC source:
https://gist.githubusercontent.com/shaos26/f514dff98be5c90da07bc3467214ef10/raw/e16e1fa984b297ef7bf18e2c8a5992fc63daa062/poc.c
Initramfs init:
https://gist.githubusercontent.com/shaos26/f514dff98be5c90da07bc3467214ef10/raw/e16e1fa984b297ef7bf18e2c8a5992fc63daa062/initramfs-init
Runner:
https://gist.githubusercontent.com/shaos26/f514dff98be5c90da07bc3467214ef10/raw/e16e1fa984b297ef7bf18e2c8a5992fc63daa062/reproduce.sh

Clone the Gist and kernel source, build the kernel with the attached
config, and run the self-contained initramfs builder/runner:

    git clone \
      https://gist.github.com/shaos26/f514dff98be5c90da07bc3467214ef10.git \
      sch-plug-repro
    cd sch-plug-repro
    git clone \
      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git \
      linux
    git -C linux checkout c03c1a14507dfaf8b856c2608f9ac216021fc222
    cp kernel-config.txt linux/.config
    make -C linux olddefconfig
    make -C linux -j"$(nproc)" bzImage
    KERNEL="$PWD/linux/arch/x86/boot/bzImage" \
      LOG=/tmp/sch-plug-positive.log sh ./reproduce.sh

The runner builds a static PoC and initramfs, then boots QEMU.  The PoC
creates a veth pair, attaches sch_plug, performs the reset/release
sequence, and records tagged frames.  It requires GCC, BusyBox, cpio,
gzip, and QEMU on the host.  The guest runs as root; the equivalent
direct trigger requires CAP_NET_ADMIN and CAP_NET_RAW.  The attached
console output contains two positive QEMU runs and patched-kernel
controls.

Fixes: fe6bea7f1f3a ("sch_plug: purge buffered packets during reset")
Signed-off-by: shaos <shawnq1@163.com>
---
 net/sched/sch_plug.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_plug.c b/net/sched/sch_plug.c
--- a/net/sched/sch_plug.c
+++ b/net/sched/sch_plug.c
@@ -202,6 +202,18 @@
 	return 0;
 }
 
+static void plug_reset(struct Qdisc *sch)
+{
+	struct plug_sched_data *q = qdisc_priv(sch);
+
+	qdisc_reset_queue(sch);
+	q->pkts_current_epoch = 0;
+	q->pkts_last_epoch = 0;
+	q->pkts_to_release = 0;
+	q->unplug_indefinite = false;
+	q->throttled = true;
+}
+
 static struct Qdisc_ops plug_qdisc_ops __read_mostly = {
 	.id          =       "plug",
 	.priv_size   =       sizeof(struct plug_sched_data),
@@ -210,7 +222,7 @@
 	.peek        =       qdisc_peek_dequeued,
 	.init        =       plug_init,
 	.change      =       plug_change,
-	.reset       =	     qdisc_reset_queue,
+	.reset       =	     plug_reset,
 	.owner       =       THIS_MODULE,
 };
 MODULE_ALIAS_NET_SCH("plug");
-- 
2.34.1

Re: [PATCH] net/sched: reset sch_plug epoch state

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net/sched: reset sch_plug epoch state

[netdev-bot+sashiko]

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

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

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] plug_reset() (net/sched/sch_plug.c:205-215) always sets
  `q->unplug_indefinite = false` and `q->throttled = true`.
- [Low] The commit log is a bug-report email pasted in as-is: - It opens
  with 'Hello,'.

--

net/sched: reset sch_plug epoch state

This patch replaces sch_plug's reset callback, qdisc_reset_queue, with a
new plug_reset(). The new function purges the queue, clears the epoch
counters, clears unplug_indefinite and sets throttled. The aim is to stop
a stale pkts_last_epoch from being used as release credit by a later
TCQ_PLUG_RELEASE_ONE after a link reset.

> Hello,

[Severity: Low]
This isn't a bug, but the commit log looks like a bug report email pasted
in as-is. It opens with a salutation and carries reporter environment
metadata:

> HEAD commit:    c03c1a14507dfaf8b856c2608f9ac216021fc222
> Git tree:       30fd2ef82912c26cf3a58ad0ba6afc375fa851ac
> Source version: Linux 7.3
> Compiler:       gcc 11.4.0, GNU ld 2.38

It also has five external Gist URLs and a multi-step clone/build/run
script. None of this describes the change itself, and the external links
may not stay available.

Could the log be cut down to a self-contained description of the problem
and the fix?

The log also doesn't mention that plug_reset() now clears
unplug_indefinite and forces throttled to true, which users can see (see
below).

> diff --git a/net/sched/sch_plug.c b/net/sched/sch_plug.c
> index b60ddfee6a68a..def95e8243ba9 100644
> --- a/net/sched/sch_plug.c
> +++ b/net/sched/sch_plug.c
> @@ -202,6 +202,18 @@ static int plug_change(struct Qdisc *sch, struct nlattr *opt,
>  	return 0;
>  }
>  
> +static void plug_reset(struct Qdisc *sch)
> +{
> +	struct plug_sched_data *q = qdisc_priv(sch);
> +
> +	qdisc_reset_queue(sch);
> +	q->pkts_current_epoch = 0;
> +	q->pkts_last_epoch = 0;
> +	q->pkts_to_release = 0;
> +	q->unplug_indefinite = false;
> +	q->throttled = true;

[Severity: Medium]
Is it intended that plug_reset() also takes the qdisc out of pass-through
mode? Fixing the stale epoch credit described in the commit message only
needs the three counters cleared.

After TCQ_PLUG_RELEASE_INDEFINITE, plug_change() already zeroes every
counter:

	case TCQ_PLUG_RELEASE_INDEFINITE:
		q->unplug_indefinite = true;
		q->pkts_to_release = 0;
		q->pkts_last_epoch = 0;
		q->pkts_current_epoch = 0;
		q->throttled = false;

plug_enqueue() doesn't count packets while unplug_indefinite is set, so
release credit can't go stale in that mode. With the old
qdisc_reset_queue callback, pass-through mode survived a reset.

With this change, a reset on carrier loss goes through this path:

linkwatch_do_dev()
  dev_deactivate(dev, true)
    dev_deactivate_many()
      dev_reset_queue()
        qdisc_reset()
          plug_reset()

The same happens on ifdown/up or a parent qdisc reset. In each case the
qdisc is left throttled with unplug_indefinite cleared. dev_activate()
reattaches the same qdisc instance and doesn't restore these fields.

>From then on plug_dequeue() returns nothing:

	if (q->throttled)
		return NULL;

plug_enqueue() counts new packets into pkts_current_epoch and buffers
them up to q->limit. After that, qdisc_drop() tail-drops the rest.

Doesn't this contradict the uAPI comment in
include/uapi/linux/pkt_sched.h?

	 * TCQ_PLUG_RELEASE_INDEFINITE: Dequeue all packets from queue.
	 *   Stop buffering packets until the next TCQ_PLUG_BUFFER
	 *   command is received (just act as a pass-thru queue).

If the controller has only ever used TCQ_PLUG_RELEASE_INDEFINITE, or has
exited, would egress traffic stay stalled after a link flap until another
release command is sent?

Would it work to leave unplug_indefinite unchanged and set
q->throttled = !q->unplug_indefinite, or to leave both fields alone?
plug_dequeue() already re-throttles on its own when pkts_to_release is 0.

The reproducer described in the commit message only exercises
TCQ_PLUG_RELEASE_ONE epoch mode, so it wouldn't catch this.

> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/6AEA1C24.11E1A7.00008%40m16.mail.163.com