Re: CVS commit: src/sys

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Fri, 15 Dec 2017 05:01:17 +0000
    From:        "Kengo NAKAHARA" <knakahara%netbsd.org@localhost>
    Message-ID:  <20171215050117.241BAFB40%cvs.NetBSD.org@localhost>

  | 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.)

kre