Revert "net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"

Bug #2011926 reported by Philip Cox
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
linux (Ubuntu)
Fix Committed
Undecided
Philip Cox
Jammy
Fix Released
Medium
Unassigned

Bug Description

Upstream commit: af7b29b1deaac6da3bb7637f0e263dfab7bfc7a3

Revert "net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"
taprio_attach() has this logic at the end, which should have been
removed with the blamed patch (which is now being reverted):

 /* access to the child qdiscs is not needed in offload mode */
 if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
  kfree(q->qdiscs);
  q->qdiscs = NULL;
 }

because otherwise, we make use of q->qdiscs[] even after this array was
deallocated, namely in taprio_leaf(). Therefore, whenever one would try
to attach a valid child qdisc to a fully offloaded taprio root, one
would immediately dereference a NULL pointer.

$ tc qdisc replace dev eno0 handle 8001: parent root taprio \
 num_tc 8 \
 map 0 1 2 3 4 5 6 7 \
 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
 max-sdu 0 0 0 0 0 200 0 0 \
 base-time 200 \
 sched-entry S 80 20000 \
 sched-entry S a0 20000 \
 sched-entry S 5f 60000 \
 flags 2
$ max_frame_size=1500
$ data_rate_kbps=20000
$ port_transmit_rate_kbps=1000000
$ idleslope=$data_rate_kbps
$ sendslope=$(($idleslope - $port_transmit_rate_kbps))
$ locredit=$(($max_frame_size * $sendslope / $port_transmit_rate_kbps))
$ hicredit=$(($max_frame_size * $idleslope / $port_transmit_rate_kbps))
$ tc qdisc replace dev eno0 parent 8001:7 cbs \
 idleslope $idleslope \
 sendslope $sendslope \
 hicredit $hicredit \
 locredit $locredit \
 offload 0

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
pc : taprio_leaf+0x28/0x40
lr : qdisc_leaf+0x3c/0x60
Call trace:
 taprio_leaf+0x28/0x40
 tc_modify_qdisc+0xf0/0x72c
 rtnetlink_rcv_msg+0x12c/0x390
 netlink_rcv_skb+0x5c/0x130
 rtnetlink_rcv+0x1c/0x2c

The solution is not as obvious as the problem. The code which deallocates
q->qdiscs[] is in fact copied and pasted from mqprio, which also
deallocates the array in mqprio_attach() and never uses it afterwards.

Therefore, the identical cleanup logic of priv->qdiscs[] that
mqprio_destroy() has is deceptive because it will never take place at
qdisc_destroy() time, but just at raw ops->destroy() time (otherwise
said, priv->qdiscs[] do not last for the entire lifetime of the mqprio
root), but rather, this is just the twisted way in which the Qdisc API
understands error path cleanup should be done (Qdisc_ops :: destroy() is
called even when Qdisc_ops :: init() never succeeded).

Side note, in fact this is also what the comment in mqprio_init() says:

 /* pre-allocate qdisc, attachment can't fail */

Or reworded, mqprio's priv->qdiscs[] scheme is only meant to serve as
data passing between Qdisc_ops :: init() and Qdisc_ops :: attach().

[ this comment was also copied and pasted into the initial taprio
  commit, even though taprio_attach() came way later ]

The problem is that taprio also makes extensive use of the q->qdiscs[]
array in the software fast path (taprio_enqueue() and taprio_dequeue()),
but it does not keep a reference of its own on q->qdiscs[i] (you'd think
that since it creates these Qdiscs, it holds the reference, but nope,
this is not completely true).

To understand the difference between taprio_destroy() and mqprio_destroy()
one must look before commit 13511704f8d7 ("net: taprio offload: enforce
qdisc to netdev queue mapping"), because that just muddied the waters.

In the "original" taprio design, taprio always attached itself (the root
Qdisc) to all netdev TX queues, so that dev_qdisc_enqueue() would go
through taprio_enqueue().

It also called qdisc_refcount_inc() on itself for as many times as there
were netdev TX queues, in order to counter-balance what tc_get_qdisc()
does when destroying a Qdisc (simplified for brevity below):

 if (n->nlmsg_type == RTM_DELQDISC)
  err = qdisc_graft(dev, parent=NULL, new=NULL, q, extack);

qdisc_graft(where "new" is NULL so this deletes the Qdisc):

 for (i = 0; i < num_q; i++) {
  struct netdev_queue *dev_queue;

  dev_queue = netdev_get_tx_queue(dev, i);

  old = dev_graft_qdisc(dev_queue, new);
  if (new && i > 0)
   qdisc_refcount_inc(new);

  qdisc_put(old);
  ~~~~~~~~~~~~~~
  this decrements taprio's refcount once for each TX queue
 }

 notify_and_destroy(net, skb, n, classid,
      rtnl_dereference(dev->qdisc), new);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      and this finally decrements it to zero,
      making qdisc_put() call qdisc_destroy()

The q->qdiscs[] created using qdisc_create_dflt() (or their
replacements, if taprio_graft() was ever to get called) were then
privately freed by taprio_destroy().

This is still what is happening after commit 13511704f8d7 ("net: taprio
offload: enforce qdisc to netdev queue mapping"), but only for software
mode.

In full offload mode, the per-txq "qdisc_put(old)" calls from
qdisc_graft() now deallocate the child Qdiscs rather than decrement
taprio's refcount. So when notify_and_destroy(taprio) finally calls
taprio_destroy(), the difference is that the child Qdiscs were already
deallocated.

And this is exactly why the taprio_attach() comment "access to the child
qdiscs is not needed in offload mode" is deceptive too. Not only the
q->qdiscs[] array is not needed, but it is also necessary to get rid of
it as soon as possible, because otherwise, we will also call qdisc_put()
on the child Qdiscs in qdisc_destroy() -> taprio_destroy(), and this
will cause a nasty use-after-free/refcount-saturate/whatever.

In short, the problem is that since the blamed commit, taprio_leaf()
needs q->qdiscs[] to not be freed by taprio_attach(), while qdisc_destroy()
-> taprio_destroy() does need q->qdiscs[] to be freed by taprio_attach()
for full offload. Fixing one problem triggers the other.

All of this can be solved by making taprio keep its q->qdiscs[i] with a
refcount elevated at 2 (in offloaded mode where they are attached to the
netdev TX queues), both in taprio_attach() and in taprio_graft(). The
generic qdisc_graft() would just decrement the child qdiscs' refcounts
to 1, and taprio_destroy() would give them the final coup de grace.

However the rabbit hole of changes is getting quite deep, and the
complexity increases. The blamed commit was supposed to be a bug fix in
the first place, and the bug it addressed is not so significant so as to
justify further rework in stable trees. So I'd rather just revert it.
I don't know enough about multi-queue Qdisc design to make a proper
judgement right now regarding what is/isn't idiomatic use of Qdisc
concepts in taprio. I will try to study the problem more and come with a
different solution in net-next.

Fixes: 1461d212ab27 ("net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs")
Reported-by: Muhammad Husaini Zulkifli <email address hidden>
Reported-by: Vinicius Costa Gomes <email address hidden>
Signed-off-by: Vladimir Oltean <email address hidden>
Reviewed-by: Vinicius Costa Gomes <email address hidden>
Link: https://<email address hidden>
Signed-off-by: Jakub Kicinski <email address hidden>

CVE References

Philip Cox (philcox)
Changed in linux (Ubuntu):
status: New → In Progress
Changed in linux (Ubuntu Jammy):
status: New → In Progress
Philip Cox (philcox)
Changed in linux (Ubuntu Jammy):
status: In Progress → Fix Committed
Changed in linux (Ubuntu):
status: In Progress → Fix Committed
Revision history for this message
Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote :

This bug is awaiting verification that the linux/5.15.0-72.79 kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-jammy' to 'verification-done-jammy'. If the problem still exists, change the tag 'verification-needed-jammy' to 'verification-failed-jammy'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: kernel-spammed-jammy-linux verification-needed-jammy
Stefan Bader (smb)
Changed in linux (Ubuntu Jammy):
importance: Undecided → Medium
Revision history for this message
Philip Cox (philcox) wrote :

Verified the fix.

tags: added: verification-done-jammy
removed: verification-needed-jammy
Revision history for this message
Launchpad Janitor (janitor) wrote :
Download full text (23.7 KiB)

This bug was fixed in the package linux - 5.15.0-72.79

---------------
linux (5.15.0-72.79) jammy; urgency=medium

  * jammy/linux: 5.15.0-72.79 -proposed tracker (LP: #2016548)

  * Add split lock detection for EMR (LP: #2015855)
    - x86/split_lock: Enumerate architectural split lock disable bit

  * selftest: fib_tests: Always cleanup before exit (LP: #2015956)
    - selftest: fib_tests: Always cleanup before exit

  * Add support for intel EMR cpu (LP: #2015372)
    - platform/x86: intel-uncore-freq: add Emerald Rapids support
    - perf/x86/intel/cstate: Add Emerald Rapids
    - perf/x86/rapl: Add support for Intel Emerald Rapids
    - intel_idle: add Emerald Rapids Xeon support
    - tools/power/x86/intel-speed-select: Add Emerald Rapid quirk
    - tools/power turbostat: Introduce support for EMR
    - powercap: intel_rapl: add support for Emerald Rapids
    - EDAC/i10nm: Add Intel Emerald Rapids server support

  * Kernel livepatch ftrace graph fix (LP: #2013603)
    - kprobes: treewide: Remove trampoline_address from
      kretprobe_trampoline_handler()
    - kprobes: treewide: Make it harder to refer kretprobe_trampoline directly
    - kprobes: Add kretprobe_find_ret_addr() for searching return address
    - s390/unwind: recover kretprobe modified return address in stacktrace
    - s390/unwind: fix fgraph return address recovery

  * Jammy update: v5.15.98 upstream stable release (LP: #2015600)
    - Linux 5.15.98

  * Jammy update: v5.15.97 upstream stable release (LP: #2015599)
    - ionic: refactor use of ionic_rx_fill()
    - Fix XFRM-I support for nested ESP tunnels
    - arm64: dts: rockchip: drop unused LED mode property from rk3328-roc-cc
    - ARM: dts: rockchip: add power-domains property to dp node on rk3288
    - HID: elecom: add support for TrackBall 056E:011C
    - ACPI: NFIT: fix a potential deadlock during NFIT teardown
    - btrfs: send: limit number of clones and allocated memory size
    - ASoC: rt715-sdca: fix clock stop prepare timeout issue
    - IB/hfi1: Assign npages earlier
    - neigh: make sure used and confirmed times are valid
    - HID: core: Fix deadloop in hid_apply_multiplier.
    - x86/cpu: Add Lunar Lake M
    - staging: mt7621-dts: change palmbus address to lower case
    - bpf: bpf_fib_lookup should not return neigh in NUD_FAILED state
    - net: Remove WARN_ON_ONCE(sk->sk_forward_alloc) from sk_stream_kill_queues().
    - vc_screen: don't clobber return value in vcs_read
    - scripts/tags.sh: Invoke 'realpath' via 'xargs'
    - scripts/tags.sh: fix incompatibility with PCRE2
    - usb: dwc3: pci: add support for the Intel Meteor Lake-M
    - USB: serial: option: add support for VW/Skoda "Carstick LTE"
    - usb: gadget: u_serial: Add null pointer check in gserial_resume
    - USB: core: Don't hold device lock while reading the "descriptors" sysfs file
    - Linux 5.15.97

  * Jammy update: v5.15.96 upstream stable release (LP: #2015595)
    - drm/etnaviv: don't truncate physical page address
    - wifi: rtl8xxxu: gen2: Turn on the rate control
    - drm/edid: Fix minimum bpc supported with DSC1.2 for HDMI sink
    - clk: mxl: Switch from direct readl/writel based IO to regmap based IO
    - ...

Changed in linux (Ubuntu Jammy):
status: Fix Committed → Fix Released
Revision history for this message
Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote :

This bug is awaiting verification that the linux-riscv-5.15/5.15.0-1034.38~20.04.1 kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-focal' to 'verification-done-focal'. If the problem still exists, change the tag 'verification-needed-focal' to 'verification-failed-focal'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: kernel-spammed-focal-linux-riscv-5.15 verification-needed-focal
Revision history for this message
Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote :

This bug is awaiting verification that the linux-intel-iotg/5.15.0-1031.36 kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-jammy' to 'verification-done-jammy'. If the problem still exists, change the tag 'verification-needed-jammy' to 'verification-failed-jammy'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: kernel-spammed-jammy-linux-intel-iotg verification-needed-jammy
removed: verification-done-jammy
Revision history for this message
Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote :

This bug is awaiting verification that the linux-aws/5.15.0-1038.43 kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-jammy' to 'verification-done-jammy'. If the problem still exists, change the tag 'verification-needed-jammy' to 'verification-failed-jammy'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: kernel-spammed-jammy-linux-aws
Revision history for this message
Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote :

This bug is awaiting verification that the linux-azure/5.15.0-1040.47 kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-jammy' to 'verification-done-jammy'. If the problem still exists, change the tag 'verification-needed-jammy' to 'verification-failed-jammy'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: kernel-spammed-jammy-linux-azure
Revision history for this message
Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote :

This bug is awaiting verification that the linux-aws-5.15/5.15.0-1046.51~20.04.1 kernel in -proposed solves the problem. Please test the kernel and update this bug with the results. If the problem is solved, change the tag 'verification-needed-focal-linux-aws-5.15' to 'verification-done-focal-linux-aws-5.15'. If the problem still exists, change the tag 'verification-needed-focal-linux-aws-5.15' to 'verification-failed-focal-linux-aws-5.15'.

If verification is not done by 5 working days from today, this fix will be dropped from the source code, and this bug will be closed.

See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you!

tags: added: kernel-spammed-focal-linux-aws-5.15-v2 verification-needed-focal-linux-aws-5.15
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.