Re: [PATCH net-next v4 1/3] net/sched: sch_fq_pie: add per-flow statistics via class ops

[Simon Horman <horms@kernel.org>]

On Mon, Jul 27, 2026 at 10:10:54PM +0530, Hemendra M. Naik wrote:

...

> +static struct tcf_block *fq_pie_tcf_block(struct Qdisc *sch, unsigned long cl,
> +					  struct netlink_ext_ack *extack)
> +{
> +	struct fq_pie_sched_data *q = qdisc_priv(sch);
> +
> +	if (cl)
> +		return NULL;
> +	return q->block;
> +}

Hi,

The AI-generated review of this patch on netdev-ai [1] reports several
findings. And while I'd appreciate it if you could look over each of them
the one below seems to warrant highlighting here.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260727164056.106203-1-hemendranaik%40gmail.com?part=1

   Is the side effect of making fq_pie filter-capable intended, and should
   it be described in the commit message?

   Before this patch fq_pie_qdisc_ops had no .cl_ops, so filter attachment
   was rejected in net/sched/cls_api.c:__tcf_qdisc_find():

	cops = (*q)->ops->cl_ops;
	if (!cops) {
		NL_SET_ERR_MSG(extack, "Qdisc not classful");
		err = -EINVAL;

   q->filter_list was therefore always NULL and the filter branch of
   fq_pie_classify() was unreachable since fq_pie was added.  With .cl_ops
   present and fq_pie_tcf_block() returning q->block, 'tc filter add ...
   parent <fq_pie handle>' now succeeds and classifier verdicts plus
   classid-driven flow selection become live in the enqueue fast path.

   That newly reachable code does not handle TC_ACT_CONSUMED:

   net/sched/sch_fq_pie.c:fq_pie_classify() {
	result = tcf_classify_qdisc(skb, filter, &res, false);
	if (result >= 0) {
		switch (result) {
		case TC_ACT_STOLEN:
		case TC_ACT_QUEUED:
		case TC_ACT_TRAP:
			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
			fallthrough;
		case TC_ACT_SHOT:
			return 0;
		}
		if (TC_H_MIN(res.classid) <= q->flows_cnt)
			return TC_H_MIN(res.classid);
	}

   TC_ACT_CONSUMED is TC_ACT_VALUE_MAX + 1, so it is >= 0 and matches no
   case arm, and control reaches the classid path.  act_ct produces it
   after the skb ownership has already been handed away:

   net/sched/act_ct.c:tcf_ct_act() {
   out_frag:
	if (err != -EINPROGRESS)
		tcf_action_inc_drop_qstats(&c->common);
	return TC_ACT_CONSUMED;

   tcf_action_exec() propagates any value != TC_ACT_PIPE and
   __tcf_classify() returns it verbatim.  For a filter such as
	tc filter add dev eth0 parent 1: matchall action ct

   with no flowid, res.classid is 0, TC_H_MIN(0) <= flows_cnt holds and 0
   is returned, so fq_pie_qdisc_enqueue() frees the consumed skb a second
   time:

	idx = fq_pie_classify(skb, sch, &ret);
	if (idx == 0) {
		if (ret & __NET_XMIT_BYPASS)
			qdisc_qstats_drop(sch);
		__qdisc_drop(skb, to_free);

   and with 'flowid 1:N' a valid flow index is returned instead, so
   flow_queue_add() links the already freed skb into q->flows[idx].  Can
   fragmented traffic hitting that qdisc then cause a double free or
   use-after-free?

   That TC_ACT_CONSUMED is expected from qdisc-attached filter chains is
   also visible in net/sched/cls_api.c:tcf_qevent_handle(), which has an
   explicit case TC_ACT_CONSUMED: arm that must not free the skb.

   fq_codel, sfq and cake have the same missing arm, but for fq_pie the
   path only becomes reachable with this patch.

...