Re: [Patch net 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0

[Cong Wang <xiyou.wangcong@gmail.com>]

On Thu, Jan 30, 2025 at 06:17:36AM -0500, Jamal Hadi Salim wrote:
> On Fri, Jan 24, 2025 at 1:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > From: Quang Le <quanglex97@gmail.com>
> >
> > Expected behaviour:
> > In case we reach scheduler's limit, pfifo_tail_enqueue() will drop a
> > packet in scheduler's queue and decrease scheduler's qlen by one.
> > Then, pfifo_tail_enqueue() enqueue new packet and increase
> > scheduler's qlen by one. Finally, pfifo_tail_enqueue() return
> > `NET_XMIT_CN` status code.
> >
> > Weird behaviour:
> > In case we set `sch->limit == 0` and trigger pfifo_tail_enqueue() on a
> > scheduler that has no packet, the 'drop a packet' step will do nothing.
> > This means the scheduler's qlen still has value equal 0.
> > Then, we continue to enqueue new packet and increase scheduler's qlen by
> > one. In summary, we can leverage pfifo_tail_enqueue() to increase qlen by
> > one and return `NET_XMIT_CN` status code.
> >
> > The problem is:
> > Let's say we have two qdiscs: Qdisc_A and Qdisc_B.
> >  - Qdisc_A's type must have '->graft()' function to create parent/child relationship.
> >    Let's say Qdisc_A's type is `hfsc`. Enqueue packet to this qdisc will trigger `hfsc_enqueue`.
> >  - Qdisc_B's type is pfifo_head_drop. Enqueue packet to this qdisc will trigger `pfifo_tail_enqueue`.
> >  - Qdisc_B is configured to have `sch->limit == 0`.
> >  - Qdisc_A is configured to route the enqueued's packet to Qdisc_B.
> >
> > Enqueue packet through Qdisc_A will lead to:
> >  - hfsc_enqueue(Qdisc_A) -> pfifo_tail_enqueue(Qdisc_B)
> >  - Qdisc_B->q.qlen += 1
> >  - pfifo_tail_enqueue() return `NET_XMIT_CN`
> >  - hfsc_enqueue() check for `NET_XMIT_SUCCESS` and see `NET_XMIT_CN` => hfsc_enqueue() don't increase qlen of Qdisc_A.
> >
> > The whole process lead to a situation where Qdisc_A->q.qlen == 0 and Qdisc_B->q.qlen == 1.
> > Replace 'hfsc' with other type (for example: 'drr') still lead to the same problem.
> > This violate the design where parent's qlen should equal to the sum of its childrens'qlen.
> >
> > Bug impact: This issue can be used for user->kernel privilege escalation when it is reachable.
> >
> > Reported-by: Quang Le <quanglex97@gmail.com>
> > Signed-off-by: Quang Le <quanglex97@gmail.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> 
> Fixes: 57dbb2d83d100 ?

Ah, probably. I will add the Fixes tag in v3 after I resolve the
selftest issue.

Thanks!