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

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

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>