From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: route delete ifp NULL check
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Mon, 4 Aug 2025 19:03:06 +0300

On Mon, Aug 04, 2025 at 01:30:42PM +0200, Alexander Bluhm wrote:
> On Mon, Aug 04, 2025 at 05:46:44AM +0300, Vitaliy Makkoveev wrote:
> > On Sun, Aug 03, 2025 at 02:26:20PM +0200, Alexander Bluhm wrote:
> > > Hi,
> > > 
> > > Coverity complains that we check ifp NULL pointer in rtrequest_delete(),
> > > but later on we dereference it unconditinally.
> > > 
> > > The only caller that might have a NULL in ifp is nd6_free().  But
> > > there we also dereference it later.  I think the exclusive net lock
> > > protects us there, but I want to assure it with a kassert.
> > > 
> > > When we unlock ND6, we have to reconsider this.
> > > 
> > > ok?
> > > 
> > 
> > Exclusive netlock protects us from `rt_ifidx' change, but not from
> > the interface detach from if_idxmap. So hypothetically `ifp' could be
> > NULL during race condition. However, seems the nd6_free() callers own
> > the `ifp' related to this `rt'. Could we pass them to the nd6_free()
> > together?
> 
> Good idea, diff below.
> 
> My only concern was that sdl->sdl_index is always the same as
> rt->rt_ifidx in nd6_purge().  I think we can assume that.
> 
> ok?
> 

ok mvs

> bluhm
> 
> Index: net/route.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> diff -u -p -r1.447 route.c
> --- net/route.c	14 Jul 2025 08:48:51 -0000	1.447
> +++ net/route.c	4 Aug 2025 10:19:18 -0000
> @@ -907,7 +907,7 @@ rtrequest_delete(struct rt_addrinfo *inf
>  		return (ESRCH);
>  
>  	/* Make sure that's the route the caller want to delete. */
> -	if (ifp != NULL && ifp->if_index != rt->rt_ifidx) {
> +	if (ifp->if_index != rt->rt_ifidx) {
>  		rtfree(rt);
>  		return (ESRCH);
>  	}
> Index: netinet6/nd6.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
> diff -u -p -r1.296 nd6.c
> --- netinet6/nd6.c	3 Aug 2025 11:08:40 -0000	1.296
> +++ netinet6/nd6.c	4 Aug 2025 11:18:49 -0000
> @@ -99,7 +99,7 @@ void nd6_slowtimo(void *);
>  void nd6_expire(void *);
>  void nd6_expire_timer(void *);
>  void nd6_invalidate(struct rtentry *);
> -void nd6_free(struct rtentry *, int);
> +void nd6_free(struct rtentry *, struct ifnet *ifp, int);
>  int nd6_llinfo_timer(struct rtentry *, int);
>  
>  struct timeout nd6_timer_to;
> @@ -329,7 +329,7 @@ nd6_llinfo_timer(struct rtentry *rt, int
>  			} else
>  				atomic_sub_int(&ln_hold_total, len);
>  
> -			nd6_free(rt, i_am_router);
> +			nd6_free(rt, ifp, i_am_router);
>  			ln = NULL;
>  		}
>  		break;
> @@ -345,7 +345,7 @@ nd6_llinfo_timer(struct rtentry *rt, int
>  	case ND6_LLINFO_PURGE:
>  		/* Garbage Collection(RFC 2461 5.3) */
>  		if (!ND6_LLINFO_PERMANENT(ln)) {
> -			nd6_free(rt, i_am_router);
> +			nd6_free(rt, ifp, i_am_router);
>  			ln = NULL;
>  		}
>  		break;
> @@ -366,7 +366,7 @@ nd6_llinfo_timer(struct rtentry *rt, int
>  			nd6_ns_output(ifp, &dst->sin6_addr, &dst->sin6_addr,
>  			    &ln->ln_saddr6, 0);
>  		} else {
> -			nd6_free(rt, i_am_router);
> +			nd6_free(rt, ifp, i_am_router);
>  			ln = NULL;
>  		}
>  		break;
> @@ -476,7 +476,7 @@ nd6_purge(struct ifnet *ifp)
>  		    rt->rt_gateway->sa_family == AF_LINK) {
>  			sdl = satosdl(rt->rt_gateway);
>  			if (sdl->sdl_index == ifp->if_index)
> -				nd6_free(rt, i_am_router);
> +				nd6_free(rt, ifp, i_am_router);
>  		}
>  	}
>  }
> @@ -638,16 +638,13 @@ nd6_invalidate(struct rtentry *rt)
>   * Free an nd6 llinfo entry.
>   */
>  void
> -nd6_free(struct rtentry *rt, int i_am_router)
> +nd6_free(struct rtentry *rt, struct ifnet *ifp, int i_am_router)
>  {
>  	struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo;
>  	struct in6_addr in6 = satosin6(rt_key(rt))->sin6_addr;
> -	struct ifnet *ifp;
>  
>  	NET_ASSERT_LOCKED_EXCLUSIVE();
>  
> -	ifp = if_get(rt->rt_ifidx);
> -
>  	if (!i_am_router) {
>  		if (ln->ln_router) {
>  			/*
> @@ -669,8 +666,6 @@ nd6_free(struct rtentry *rt, int i_am_ro
>  	 */
>  	if (!ISSET(rt->rt_flags, RTF_STATIC|RTF_CACHED))
>  		rtdeletemsg(rt, ifp, ifp->if_rdomain);
> -
> -	if_put(ifp);
>  }
>  
>  /*
> @@ -1054,7 +1049,7 @@ nd6_cache_lladdr(struct ifnet *ifp, cons
>  		return;
>  	if ((rt->rt_flags & (RTF_GATEWAY | RTF_LLINFO)) != RTF_LLINFO) {
>  fail:
> -		nd6_free(rt, i_am_router);
> +		nd6_free(rt, ifp, i_am_router);
>  		rtfree(rt);
>  		return;
>  	}