ifaddr refcount problem

[Navdeep Parhar]

I got distracted by some other issues and lost track of this thread.
Can one of you please commit the fix if the discussion has reached a
conclusion?  Thanks!

Regards,
Navdeep

On Wed, Jun 25, 2014 at 02:08:35PM +0400, Gleb Smirnoff wrote:
>   Alan,
> 
> On Tue, Jun 24, 2014 at 08:43:40AM -0600, Alan Somers wrote:
> A> That looks better.  But I think there is one more possibility for a
> A> leak.  For multicast packets, IFP_TO_IA at line 263 will call
> A> ifa_ref(), unless the the interface has no address assigned.  How
> A> about this patch instead?  Also, not tested.
> 
> Again you are right, thanks.
> 
> The patch needs to reset have_ia_ref in the 'goto again' case.
> 
> Also, I'd suggest a tiny change to your patch so that we don't
> have a broken logic reading the code as "I have ia reference,
> but don't have ia".
> 
> -- 
> Totus tuus, Glebius.

> Index: ip_output.c
> ===================================================================
> --- ip_output.c	(revision 267536)
> +++ ip_output.c	(working copy)
> @@ -136,6 +136,7 @@ ip_output(struct mbuf *m, struct mbuf *opt, struct
>  	struct rtentry *rte;	/* cache for ro->ro_rt */
>  	struct in_addr odst;
>  	struct m_tag *fwd_tag = NULL;
> +	int have_ia_ref;
>  #ifdef IPSEC
>  	int no_route_but_check_spd = 0;
>  #endif
> @@ -202,6 +203,7 @@ ip_output(struct mbuf *m, struct mbuf *opt, struct
>  	gw = dst = (struct sockaddr_in *)&ro->ro_dst;
>  again:
>  	ia = NULL;
> +	have_ia_ref = 0;
>  	/*
>  	 * If there is a cached route, check that it is to the same
>  	 * destination and is still up.  If not, free it and try again.
> @@ -238,6 +240,7 @@ again:
>  			error = ENETUNREACH;
>  			goto bad;
>  		}
> +		have_ia_ref = 1;
>  		ip->ip_dst.s_addr = INADDR_BROADCAST;
>  		dst->sin_addr = ip->ip_dst;
>  		ifp = ia->ia_ifp;
> @@ -250,6 +253,7 @@ again:
>  			error = ENETUNREACH;
>  			goto bad;
>  		}
> +		have_ia_ref = 1;
>  		ifp = ia->ia_ifp;
>  		ip->ip_ttl = 1;
>  		isbroadcast = in_broadcast(dst->sin_addr, ifp);
> @@ -261,6 +265,8 @@ again:
>  		 */
>  		ifp = imo->imo_multicast_ifp;
>  		IFP_TO_IA(ifp, ia);
> +		if (ia)
> +			have_ia_ref = 1;
>  		isbroadcast = 0;	/* fool gcc */
>  	} else {
>  		/*
> @@ -552,8 +558,11 @@ sendit:
>  #endif
>  			error = netisr_queue(NETISR_IP, m);
>  			goto done;
> -		} else
> +		} else {
> +			if (have_ia_ref)
> +				ifa_free(&ia->ifa);
>  			goto again;	/* Redo the routing table lookup. */
> +		}
>  	}
>  
>  	/* See if local, if yes, send it to netisr with IP_FASTFWD_OURS. */
> @@ -582,6 +591,8 @@ sendit:
>  		m->m_flags |= M_SKIP_FIREWALL;
>  		m->m_flags &= ~M_IP_NEXTHOP;
>  		m_tag_delete(m, fwd_tag);
> +		if (have_ia_ref)
> +			ifa_free(&ia->ifa);
>  		goto again;
>  	}
>  
> @@ -694,6 +705,8 @@ passout:
>  done:
>  	if (ro == &iproute)
>  		RO_RTFREE(ro);
> +	if (have_ia_ref)
> +		ifa_free(&ia->ifa);
>  	return (error);
>  bad:
>  	m_freem(m);