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

[Justin Iurman <justin.iurman@gmail.com>]

On 4/4/26 13:44, Eric Dumazet wrote:
> 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)

Makes sense, I agree. But then rate-limiting would be redundant with the 
frequency of IOAM insertion, which acts as a rate-limit on its own 
already. IOAM is not (never ever) enabled by default, it must be enabled 
on ingress interface(s) explicitly, and configured accordingly. So it 
rules out the need to enforce anything on "hosts", as it is assumed you 
know what you're doing when you enable something (***). Also, IOAM only 
runs inside a limited domain (i.e., private/administrative domain 
operated by the same person/entity). Traffic must be filtered on the 
ingress of the domain (i.e., no IOAM injection from the outside), on the 
egress of the domain (i.e., prevent potential data leaking due to, e.g., 
misconfiguration), and the frequency of IOAM insertion actually is the 
rate-limit inside the domain (to each their own configuration, depending 
on performance). This is why I believe that documenting the whole thing 
would be enough in this case, since we already have rate-limiting thanks 
to the frequency of IOAM insertion.

(***) well, we may add another security barrier, just in case someone 
enables IOAM by mistake, by disabling the queue depth by default.