| Message ID | 20250416102427.3219655-1-victor@mojatatu.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net_sched: Adapt qdiscs for reentrant enqueue cases | expand |
On Wed, 16 Apr 2025 07:24:22 -0300 Victor Nogueira wrote: > As described in Gerrard's report [1], there are cases where netem can > make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets, > qfq) break whenever the enqueue callback has reentrant behaviour. > This series addresses these issues by adding extra checks that cater for > these reentrant corner cases. This series has passed all relevant test > cases in the TDC suite. Sorry for asking this question a bit late, but reentrant enqueue seems error prone. Is there a clear use case for netem as a child? If so should we also add some sort of "capability" to avoid new qdiscs falling into the same trap, without giving the problem any thought?
On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote: > As described in Gerrard's report [1], there are cases where netem can > make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets, > qfq) break whenever the enqueue callback has reentrant behaviour. > This series addresses these issues by adding extra checks that cater for > these reentrant corner cases. This series has passed all relevant test > cases in the TDC suite. > > [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/ > I am wondering why we need to enqueue the duplicate skb before enqueuing the original skb in netem? IOW, why not just swap them? Thanks
On 4/17/25 13:07, Jakub Kicinski wrote: > On Wed, 16 Apr 2025 07:24:22 -0300 Victor Nogueira wrote: >> As described in Gerrard's report [1], there are cases where netem can >> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets, >> qfq) break whenever the enqueue callback has reentrant behaviour. >> This series addresses these issues by adding extra checks that cater for >> these reentrant corner cases. This series has passed all relevant test >> cases in the TDC suite. > > Sorry for asking this question a bit late, but reentrant enqueue seems > error prone. Is there a clear use case for netem as a child? We discussed this internally as well before i sent this fix. The 3 examples are buggy in picking the correct active class in the corner case the poc produced. So these are bug fixes regardless - and they happen to fix the issue. We also wondered why it was not appropriate for netem to always be root qdisc. Looking around the manpage states that you can have a qdisc like tbf as a parent. This issue happens only when netem "duplication feature" is used. Not sure what other use cases are out there that expect netem to be a child. +Cc Stephen who can give a more authoritative answer. > If so should we also add some sort of "capability" to avoid new qdiscs > falling into the same trap, without giving the problem any thought? I think the bug needs to be fix regardless, but I think your suggestion also makes sense and is more future proof. I can send another patch later that adds a "capability",as you suggested, (let's say ALLOWS_REENTRANT_ENQ or something similar) and only allows netem to be a child of qdiscs that have this flag.
On 4/17/25 16:23, Cong Wang wrote: > On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote: >> As described in Gerrard's report [1], there are cases where netem can >> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets, >> qfq) break whenever the enqueue callback has reentrant behaviour. >> This series addresses these issues by adding extra checks that cater for >> these reentrant corner cases. This series has passed all relevant test >> cases in the TDC suite. >> >> [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/ >> > > I am wondering why we need to enqueue the duplicate skb before enqueuing > the original skb in netem? IOW, why not just swap them? I thought of doing what, I think, you are suggesting, but I was afraid of breaking netem. Stephen any comments? cheers, Victor
As described in Gerrard's report [1], there are cases where netem can make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets, qfq) break whenever the enqueue callback has reentrant behaviour. This series addresses these issues by adding extra checks that cater for these reentrant corner cases. This series has passed all relevant test cases in the TDC suite. [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/ v1 -> v2: - Removed RFC tag - Added Jamal's Acked-by - Added TDC tests - Small cleanups Victor Nogueira (5): net_sched: drr: Fix double list add in class with netem as child qdisc net_sched: hfsc: Fix a UAF vulnerability in class with netem as child qdisc net_sched: ets: Fix double list add in class with netem as child qdisc net_sched: qfq: Fix double list add in class with netem as child qdisc selftests: tc-testing: Add TDC tests that exercise reentrant enqueue behaviour net/sched/sch_drr.c | 7 +- net/sched/sch_ets.c | 7 +- net/sched/sch_hfsc.c | 2 +- net/sched/sch_qfq.c | 8 + .../tc-testing/tc-tests/infra/qdiscs.json | 148 ++++++++++++++++++ 5 files changed, 169 insertions(+), 3 deletions(-)