ACK/Cmnt: [PATCH 1/1] [SRU][Jammy] Revert "net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs"
Stefan Bader
stefan.bader at canonical.com
Tue Mar 21 10:09:44 UTC 2023
On 17.03.23 20:15, Philip Cox wrote:
> From: Vladimir Oltean <vladimir.oltean at nxp.com>
>
>
> BugLink: https://bugs.launchpad.net/bugs/2011926
>
> 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 at 0 1 at 1 1 at 2 1 at 3 1 at 4 1 at 5 1 at 6 1 at 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 <muhammad.husaini.zulkifli at intel.com>
> Reported-by: Vinicius Costa Gomes <vinicius.gomes at intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean at nxp.com>
> Reviewed-by: Vinicius Costa Gomes <vinicius.gomes at intel.com>
> Link: https://lore.kernel.org/r/20221004220100.1650558-1-vladimir.oltean@nxp.com
> Signed-off-by: Jakub Kicinski <kuba at kernel.org>
> (cherry picked from commit af7b29b1deaac6da3bb7637f0e263dfab7bfc7a3)
> Signed-off-by: Philip Cox <philip.cox at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
Same actually has been done for v5.15.96 upstream.
commit 80569627ce46b54597ee35414ba7d0a083abf53b
-Stefan
> net/sched/sch_taprio.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 135ea8b3816f..e203deacc953 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1950,14 +1950,12 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
>
> static struct Qdisc *taprio_leaf(struct Qdisc *sch, unsigned long cl)
> {
> - struct taprio_sched *q = qdisc_priv(sch);
> - struct net_device *dev = qdisc_dev(sch);
> - unsigned int ntx = cl - 1;
> + struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
>
> - if (ntx >= dev->num_tx_queues)
> + if (!dev_queue)
> return NULL;
>
> - return q->qdiscs[ntx];
> + return dev_queue->qdisc_sleeping;
> }
>
> static unsigned long taprio_find(struct Qdisc *sch, u32 classid)
--
- Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xE8675DEECBEECEA3.asc
Type: application/pgp-keys
Size: 44613 bytes
Desc: OpenPGP public key
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20230321/04ddd381/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20230321/04ddd381/attachment-0001.sig>
More information about the kernel-team
mailing list