From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: route reference leak
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 8 Apr 2024 20:38:08 +0300

> On 6 Apr 2024, at 18:18, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> Hi,
> 
> We are leaking a route reference counter in ip_output() and
> ip6_output().
> 
> If no struct route was passed to ip_output(), it uses its own iproute
> on the stack.  If that is the case, we have to free any route entry
> that we put into the local route cache.
> 
> In the pf reroute case, we reset struct route to NULL.  Before that,
> free the route entry if it was allocated locally on the stack.
> 
> ok?
> 

It’s safe to pass NULL to rtfree(), you don't need to check
ro->ro_rt.

ok mvs@

> bluhm
> 
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> diff -u -p -r1.396 ip_output.c
> --- netinet/ip_output.c	22 Feb 2024 14:25:58 -0000	1.396
> +++ netinet/ip_output.c	6 Apr 2024 14:49:07 -0000
> @@ -417,6 +417,8 @@ sendit:
> 	else if (m->m_pkthdr.pf.flags & PF_TAG_REROUTE) {
> 		/* tag as generated to skip over pf_test on rerun */
> 		m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
> +		if (ro == &iproute && ro->ro_rt)
> +			rtfree(ro->ro_rt);
> 		ro = NULL;
> 		if_put(ifp); /* drop reference since target changed */
> 		ifp = NULL;
> Index: netinet6/ip6_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> diff -u -p -r1.288 ip6_output.c
> --- netinet6/ip6_output.c	28 Feb 2024 10:57:20 -0000	1.288
> +++ netinet6/ip6_output.c	6 Apr 2024 14:49:07 -0000
> @@ -635,6 +635,8 @@ reroute:
> 		/* tag as generated to skip over pf_test on rerun */
> 		m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
> 		finaldst = ip6->ip6_dst;
> +		if (ro == &iproute && ro->ro_rt)
> +			rtfree(ro->ro_rt);
> 		ro = NULL;
> 		if_put(ifp); /* drop reference since destination changed */
> 		ifp = NULL;
>