On 15 December 2017 at 14:24, Robert Elz <k...@munnari.oz.au> wrote: > > Date: Fri, 15 Dec 2017 05:01:17 +0000 > From: "Kengo NAKAHARA" <knakah...@netbsd.org> > Message-ID: <20171215050117.241baf...@cvs.netbsd.org> > > | Fix pullup'ed mbuf leaks. > | The match function just requires enough mbuf length. > > This is still not correct. There are no mbuf leaks, I think you did > not understand maxv's point (if I remember it correctly). > > The problem isn't lost mbufs, it is that if code does a m_pullup() > (or m_ensure_contig() which is essentially the same thing) the > mbuf chain can be altered. > > What that means is that after a call of in_l2tp_match(m, ...) in the > calling function, the value of m has been lost - using the old one is > incorrect and anything might happen (up to and including a panic). > > This is why so many mbuf functions pass a struct mbuf ** instead of a > struct mbuf * so when the parameter is changed, it gets passed back to > the caller. This function probably needs something like that (the other > way is returning the updated m value, but I don't think that fits here.) > > Unfortunately, because of the way it gets called, this might not be an > easy change to make. > > The problem occurs way back in encap4_input() which does > > match = encap4_lookup(m,...); > > and then later > > encap_fillarg(m, match); > or > m_freem(m); > or > rip_input(m, off, proto); > > all of which use the 'm' that was passed to encap4_lookup(). > > encap4_lookup does .. > > prio = (*ep->func)(m, off, proto, ep->arg); > > where *ep->func is in_l2tp_match which does the m_pullup() or now > m_ensure_contig() instead which potentially changes what 'm' should > be (the old one may have been freed and replaced by a different one.)
Just wondering, is there scope for a (rather heavy) debug option in which m_ensure_contig() always returns a new mbuf for valid input? David