On Sat, Aug 02, 2025 at 11:29:18AM +0200, Alexander Bluhm wrote:
> On Sat, Aug 02, 2025 at 06:19:20AM +0300, Vitaliy Makkoveev wrote:
> > On Fri, Aug 01, 2025 at 09:43:02PM +0200, Jan Klemkow wrote:
> > > bluhm pointed out a double m_freem() and refcnt leak in veb(4).  The
> > > following diff fixes the issues.
> > > 
> > > Index: net/if_veb.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/net/if_veb.c,v
> > > diff -u -p -r1.41 if_veb.c
> > > --- net/if_veb.c	7 Jul 2025 02:28:50 -0000	1.41
> > > +++ net/if_veb.c	1 Aug 2025 19:33:33 -0000
> > > @@ -1027,8 +1027,10 @@ veb_broadcast(struct veb_softc *sc, stru
> > >  		if (veb_rule_filter(tp, VEB_RULE_LIST_OUT, m0, src, dst))
> > >  			continue;
> > >  
> > > -		if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL)
> > > -			goto done;
> > > +		if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL) {
> > > +			refcnt_rele_wake(&pm->m_refs);
> > > +			return;
> > > +		}
> > >  
> > >  		m = m_dup_pkt(m0, max_linkhdr + ETHER_ALIGN, M_NOWAIT);
> > >  		if (m == NULL) {
> > 
> > The first hunk is right except it does not fix doble m_freem() because
> > m_freem() accepts NULL. However, `pm' leaks. I would prefer "goto rele"
> > instead of "rele_wake and return", but feel free to commit as is.
> > 
> >                if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL)
> >                         goto rele;
> >                /* ... */
> >         }
> > rele:
> >         refcnt_rele_wake(&pm->m_refs);
> > done:
> >         m_freem(m0);
> > 
> > > @@ -1083,7 +1085,7 @@ veb_transmit(struct veb_softc *sc, struc
> > >  	    m->m_pkthdr.len);
> > >  
> > >  	if ((m = veb_offload(ifp, ifp0, m)) == NULL)
> > > -		goto drop;
> > > +		return (NULL);
> > >  
> > >  	(*tp->p_enqueue)(ifp0, m); /* XXX count error */
> > >  
> > >
> > 
> > m_freem() accepts NULL, I see no problems here:
> 
> You are right.  I was confused by the other diff from jan@ where
> was actually a mbuf leak.  But at least I saw the ref leak.
> 
> I would say your suggestion with "goto rele" is the nicest fix.

ok?

Thanks,
Jan

Index: net/if_veb.c
===================================================================
RCS file: /cvs/src/sys/net/if_veb.c,v
diff -u -p -r1.41 if_veb.c
--- net/if_veb.c	7 Jul 2025 02:28:50 -0000	1.41
+++ net/if_veb.c	2 Aug 2025 09:44:48 -0000
@@ -1028,7 +1028,7 @@ veb_broadcast(struct veb_softc *sc, stru
 			continue;
 
 		if ((m0 = veb_offload(ifp, ifp0, m0)) == NULL)
-			goto done;
+			goto rele;
 
 		m = m_dup_pkt(m0, max_linkhdr + ETHER_ALIGN, M_NOWAIT);
 		if (m == NULL) {
@@ -1038,6 +1038,7 @@ veb_broadcast(struct veb_softc *sc, stru
 
 		(*tp->p_enqueue)(ifp0, m); /* XXX count error */
 	}
+ rele:
 	refcnt_rele_wake(&pm->m_refs);
 
 done: