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

Justin Iurman
2026-04-02T18:10:22Z

On 4/2/26 12:17, Eric Dumazet wrote:
> We need to check __in6_dev_get() for possible NULL value, as
> suggested by Yiming Qian.
>
> Also add skb_dst_dev_rcu() instead of skb_dst_dev(),
> and two missing READ_ONCE().
>
> Note that @dev can't be NULL.
>
> Fixes: 9ee11f0fff20 ("ipv6: ioam: Data plane support for Pre-allocated Trace")
> Reported-by: Yiming Qian <yimingqian591@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Justin Iurman <justin.iurman@gmail.com>
> ---
> net/ipv6/ioam6.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/net/ipv6/ioam6.c b/net/ipv6/ioam6.c
> index 3978773bec424890cd18db78cf7cac9d3d652130..05a0b7d7e2aac35f634641fc4a791d1965dc85fd 100644
> --- a/net/ipv6/ioam6.c
> +++ b/net/ipv6/ioam6.c
> @@ -710,7 +710,9 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
> struct ioam6_schema *sc,
> unsigned int sclen, bool is_input)
> {
> - struct net_device *dev = skb_dst_dev(skb);
> + /* Note: skb_dst_dev_rcu() can't be NULL at this point. */
> + struct net_device *dev = skb_dst_dev_rcu(skb);
> + struct inet6_dev *i_skb_dev, *idev;
> struct timespec64 ts;
> ktime_t tstamp;
> u64 raw64;
> @@ -721,13 +723,16 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
>
> data = trace->data + trace->remlen * 4 - trace->nodelen * 4 - sclen * 4;
>
> + i_skb_dev = skb->dev ? __in6_dev_get(skb->dev) : NULL;
> + idev = __in6_dev_get(dev);
> +
> /* hop_lim and node_id */
> if (trace->type.bit0) {
> byte = ipv6_hdr(skb)->hop_limit;
> if (is_input)
> byte--;
>
> - raw32 = dev_net(dev)->ipv6.sysctl.ioam6_id;
> + raw32 = READ_ONCE(dev_net(dev)->ipv6.sysctl.ioam6_id);
>
> *(__be32 *)data = cpu_to_be32((byte << 24) | raw32);
> data += sizeof(__be32);
> @@ -735,18 +740,18 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
>
> /* ingress_if_id and egress_if_id */
> if (trace->type.bit1) {
> - if (!skb->dev)
> + if (!i_skb_dev)
> raw16 = IOAM6_U16_UNAVAILABLE;
> else
> - raw16 = (__force u16)READ_ONCE(__in6_dev_get(skb->dev)->cnf.ioam6_id);
> + raw16 = (__force u16)READ_ONCE(i_skb_dev->cnf.ioam6_id);
>
> *(__be16 *)data = cpu_to_be16(raw16);
> data += sizeof(__be16);
>
> - if (dev->flags & IFF_LOOPBACK)
> + if ((dev->flags & IFF_LOOPBACK) || !idev)
> raw16 = IOAM6_U16_UNAVAILABLE;
> else
> - raw16 = (__force u16)READ_ONCE(__in6_dev_get(dev)->cnf.ioam6_id);
> + raw16 = (__force u16)READ_ONCE(idev->cnf.ioam6_id);
>
> *(__be16 *)data = cpu_to_be16(raw16);
> data += sizeof(__be16);
> @@ -822,7 +827,7 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
> if (is_input)
> byte--;
>
> - raw64 = dev_net(dev)->ipv6.sysctl.ioam6_id_wide;
> + raw64 = READ_ONCE(dev_net(dev)->ipv6.sysctl.ioam6_id_wide);
>
> *(__be64 *)data = cpu_to_be64(((u64)byte << 56) | raw64);
> data += sizeof(__be64);
> @@ -830,18 +835,18 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
>
> /* ingress_if_id and egress_if_id (wide) */
> if (trace->type.bit9) {
> - if (!skb->dev)
> + if (!i_skb_dev)
> raw32 = IOAM6_U32_UNAVAILABLE;
> else
> - raw32 = READ_ONCE(__in6_dev_get(skb->dev)->cnf.ioam6_id_wide);
> + raw32 = READ_ONCE(i_skb_dev->cnf.ioam6_id_wide);
>
> *(__be32 *)data = cpu_to_be32(raw32);
> data += sizeof(__be32);
>
> - if (dev->flags & IFF_LOOPBACK)
> + if ((dev->flags & IFF_LOOPBACK) || !idev)
> raw32 = IOAM6_U32_UNAVAILABLE;
> else
> - raw32 = READ_ONCE(__in6_dev_get(dev)->cnf.ioam6_id_wide);
> + raw32 = READ_ONCE(idev->cnf.ioam6_id_wide);
>
> *(__be32 *)data = cpu_to_be32(raw32);
> data += sizeof(__be32);

LGTM, thanks Eric.

Reviewed-by: Justin Iurman <justin.iurman@gmail.com>