Re: [PATCH net v2] ipv6: rpl: add NULL check for idev in ipv6_rpl_srh_rcv()

[Andrea Mayer <andrea.mayer@uniroma2.it>]

On Mon, 18 May 2026 08:18:56 -0600
David Ahern <dsahern@kernel.org> wrote:

> On 5/18/26 8:06 AM, Andrea Mayer wrote:
> > diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> > index 03cbce842c1a..a4af6e63349c 100644
> > --- a/net/ipv6/exthdrs.c
> > +++ b/net/ipv6/exthdrs.c
> > @@ -499,6 +499,10 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
> >  	u32 r;
> >  
> >  	idev = __in6_dev_get(skb->dev);
> > +	if (!idev) {
> > +		kfree_skb_reason(skb, SKB_DROP_REASON_IPV6DISABLED);
> > +		return -1;
> > +	}
> >  
> >  	accept_rpl_seg = min(READ_ONCE(net->ipv6.devconf_all->rpl_seg_enabled),
> >  			     READ_ONCE(idev->cnf.rpl_seg_enabled));
> 
> ipv6_rpl_srh_rcv and ipv6_srh_rcv are both called by ipv6_rthdr_rcv and
> both of these functions check idev. Moving the check to ipv6_rthdr_rcv
> which already has an idev lookup would simplifying both paths -- and set
> the drop code reason the same.

Hi David,

thanks for the review. I went through the code to plan v3, and I'd like to
share a couple of points before sending it, in case I'm missing something.

ipv6_rthdr_rcv() seems to already tolerate idev == NULL. The per-device
accept_source_route read is wrapped in "if (idev)", and all
__IP6_INC_STATS() calls go through _DEVINC(), which is NULL safe.
The code after the switch:

  [...]
  switch (hdr->type) {
  case IPV6_SRCRT_TYPE_4:
      return ipv6_srh_rcv(skb);
  case IPV6_SRCRT_TYPE_3:
      return ipv6_rpl_srh_rcv(skb);
  default:
      break;
  }
  [...]

falls back to the "default: break;" path, which only uses idev through
those macros. For this reason, I think an "if (!idev) drop" at the top
would change the behavior of the default path. So if we want to put the
check on idev in this function, the check would need to go inside the two
switch cases.

Both the callees in the switch need idev to read the per-device sysctl
(idev->cnf.seg6_enabled, idev->cnf.rpl_seg_enabled). To remove their own
__in6_dev_get() they would have to receive idev from the caller.

Two possible shapes for v3, both pass idev to the callees and remove their
own __in6_dev_get() and NULL check:

(a) Check inside the two switch cases with goto to a "disabled:" label at
    the end of the function (same style as the existing "unknown_rh:"):

      switch (hdr->type) {
      case IPV6_SRCRT_TYPE_4:
          if (!idev)
              goto disabled;
          return ipv6_srh_rcv(skb, idev);
      case IPV6_SRCRT_TYPE_3:
          if (!idev)
              goto disabled;
          return ipv6_rpl_srh_rcv(skb, idev);
      default:
          break;
      }
      [...]
      disabled:
          kfree_skb_reason(skb, SKB_DROP_REASON_IPV6DISABLED);
          return -1;

(b) Single check before the switch, only for the two types that need
    idev:

      if (!idev && (hdr->type == IPV6_SRCRT_TYPE_4 ||
                    hdr->type == IPV6_SRCRT_TYPE_3)) {
          kfree_skb_reason(skb, SKB_DROP_REASON_IPV6DISABLED);
          return -1;
      }
      /* switch unchanged, idev passed to the callees */

Both change the signatures of ipv6_srh_rcv() and ipv6_rpl_srh_rcv().

Any preference, or a different approach in mind?

Thanks,
Andrea