Re: [PATCH net] ipv6: ioam: fix potential NULL dereferences in __ioam6_fill_trace_data()

Eric Dumazet
2026-04-04T11:44:53Z

On Sat, Apr 4, 2026 at 3:13 AM Justin Iurman <justin.iurman@gmail.com> wrote:
>
> On 4/3/26 23:50, Jakub Kicinski wrote:
> > On Fri, 3 Apr 2026 14:47:32 -0700 Eric Dumazet wrote:
> >>> If the ingress device has more RX queues than the egress device (dev) has
> >>> TX queues, skb_get_queue_mapping(skb) will exceed dev->num_tx_queues.
> >>>
> >>> Since skb_get_tx_queue() does not clamp the index, will it return an
> >>> out-of-bounds pointer into the egress device's dev->_tx array, causing
> >>> dereferencing queue->qdisc to read invalid memory?
> >>
> >> This seems a different bug ?
> >>
> >> I mean, do we need to fix all bug in a single patch ?
> >
> > no no, sorry, i'm just sending this out for Justin or you as a separate
> > thing.
>
> Thanks Jakub, I don't know how I missed that. It's not just about a
> possible OOB: it's actually incorrect for IOAM to do that because we're
> relying on a lucky assumption at best (i.e., if queue_mapping on RX ==
> queue_mapping on TX). This unfortunately rules out the possibility to
> have an accurate per queue visibility (which I originally provided for
> flexibility). I've had a local patch for a long time to aggregate the
> queue depth (i.e., sum of all TX queues). It is more expensive(***), but
> it would kill two birds with one stone and solve both problems
> simultaneously.
>
> Jakub, Eric, please let me know if the following plan works for both of you:
> - (net) fix the OOB reported by Jakub (and, while at it, add missing
> locks around qdisc_qstats_qlen_backlog() -> also included in my local patch)
> - (net-next) after the merge window, add the new feature
>
> After that, the per queue visibility would be considered
> legacy/deprecated and not the default behavior. Documentation would be
> updated accordingly to explain why per queue visibility cannot be trusted.
>
> (***) the advise to operators in such a case is (obviously) to reduce
> the IOAM insertion (with the frequency parameter), especially at line rate.

Getting qdisc stats can be expensive for qdisc with per-cpu storage
for counters like pfifo-fast.

If your plan is to sum MQ stats, this could have quadratic behavior :
O(nr_cpus ^ 2)

I suggest rate-limiting this very expensive operation, or OAM will
become a nice DOS tool.
(Ie not depend on operators being nice, enforce this on the hosts)