kern/60985: m_makewritable woes

["campbell+netbsd%mumble.net@localhost via gnats" <gnats-admin%NetBSD.org@localhost>]

>Number:         60985
>Category:       kern
>Synopsis:       m_makewritable woes
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Nov 09 10:10:00 +0000 2026
>Originator:     Taylor R Campbell
>Release:        current, 11, 10, ...
>Organization:
The m_writers' Guild, Inc.
>Environment:
>Description:

	Summary:

	1. m_makewritable(9) doesn't make the result contiguous
	2. various callers assume it does
	3. not clear whether m_pullup(9) makes the result writable
	4. if m_pullup and m_makewritable must be combined, not clear
	   in which order


	m_makewritable(9) was added in 2004 to support some
	copy-on-write semantics, presumably as a performance
	optimization for some mbuf operations so that parts of mbuf
	chains can be shared in copies.

	https://mail-index.netbsd.org/source-changes/2004/09/06/msg152408.html

	It is explicitly documented _not_ to make the specified range
	contiguous:

     m_makewritable(struct mbuf **mp, int off, int len, int how)
              Rearranges an mbuf chain so that len bytes from offset off are
              writable.  When it meets read-only mbufs, it allocates new
              mbufs, adjusts the chain as m_copyback_cow() does, and copies
              the original content into them.  m_makewritable() does not
              guarantee that all len bytes at off are consecutive.  [...]

	This was added by dyoung@ in 2006 with a commit message that
	makes me suspect he had recently been bitten by this:

> Let there be no mistake, m_makewritable() does not guarantee that
> the writable bytes [off, off+len) are consecutive.

	https://mail-index.netbsd.org/source-changes/2006/07/28/msg205537.html

	(However, I didn't see any recent commits by dyoung@ that
	explain exactly what might have bitten him.)

	In contrast, m_pullup(9) is guaranteed to make the first len
	bytes contiguous:

     m_pullup(struct mbuf *m, int len)
              Rearranges an mbuf chain so that len bytes are contiguous and in
              the data area of an mbuf (so that mtod() will work for a
              structure of size len).  [...]

	But it's not clear whether m_pullup is guaranteed to make those
	first len bytes writable.

	We have various callers of m_makewritable in tree that use mtod
	immediately afterward:

    209 	int error = m_makewritable(mp, 0, M_COPYALL, M_DONTWAIT);
...
    217 	ip = mtod(*mp, struct ip *);

	https://nxr.netbsd.org/xref/src/sys/external/bsd/ipf/netinet/ip_fil_netbsd.c?r=1.40#203

   1896 	if (m->m_len < sizeof(*evl) &&
   1897 	    (m = m_pullup(m, sizeof(*evl))) == NULL) {
...
   1899 	}
   1900 
   1901 	if (m_makewritable(&m, 0, sizeof(*evl), M_DONTWAIT)) {
...
   1904 	}
   1905 
   1906 	evl = mtod(m, struct ether_vlan_header *);

	https://nxr.netbsd.org/xref/src/sys/net/if_ethersubr.c?r=1.336#1896

    245 	const int tlen = off + len;
...
    254 	if (m_makewritable(&m, 0, tlen, M_NOWAIT)) {
...
    262 	}
    263 	nbuf->nb_mbuf = m;
    264 	nbuf->nb_nptr = mtod(m, uint8_t *) + off;

	https://nxr.netbsd.org/xref/src/sys/net/npf/npf_mbuf.c?r=1.25#245

    398 /*
    399  * Insure there is sufficient contiguous space to encapsulate the
    400  * 802.11 data frame.  If room isn't already there, arrange for it.
...
    406  */
    407 static struct mbuf *
    408 ieee80211_mbuf_adjust(struct ieee80211com *ic, int hdrsize,
    409 	struct ieee80211_key *key, struct mbuf *m)
    410 {
...
    482 	if (wlen != 0 && m_makewritable(&m, 0, wlen, M_DONTWAIT) != 0) {
...
    485 	}
    486 
    487 	return m;

	https://nxr.netbsd.org/xref/src/sys/net80211/ieee80211_output.c#398

    591 	m = ieee80211_mbuf_adjust(ic, hdrsize, key, m);
    592 	if (m == NULL) {
...
    595 	}
    596 
    597 	/* NB: this could be optimized because of ieee80211_mbuf_adjust */
    598 	m_adj(m, sizeof(struct ether_header) - sizeof(struct llc));
    599 	llc = mtod(m, struct llc *);

	https://nxr.netbsd.org/xref/src/sys/net80211/ieee80211_output.c#591

	(I don't think m_adj(m, len) is guaranteed to make anything
	contiguous either.)

    797 	if (__predict_false(m_makewritable(&m, 0, m->m_pkthdr.len, M_DONTWAIT)))
    798 		goto out;
    799 	ah = mtod(m, struct arphdr *);

	https://nxr.netbsd.org/xref/src/sys/netinet/if_arp.c?r=1.321#797

	^ This part was the subject of trouble after fixes for:
	PR kern/60491: arp creates state for invalid messages
	https://gnats.NetBSD.org/60491

    391 	if (__predict_false(
    392 	    m_makewritable(&m, 0, sizeof(struct ip6_hdr), M_DONTWAIT))) {
    393 		IP6_STATINC(IP6_STAT_IDROPPED);
    394 		goto bad;
    395 	}
    396 	ip6 = mtod(m, struct ip6_hdr *);

	https://nxr.netbsd.org/xref/src/sys/netinet6/ip6_input.c?r=1.228#391

>How-To-Repeat:

	lift up a rock and watch the bugs scurry around

>Fix:

	1. Determine whether it is reasonable for m_makewritable to not
	   make the result contiguous.  (There are other callers than
	   the ones quoted above, e.g. in netipsec which don't use
	   mtod; maybe non-contiguous results are fine for those.)

	2. Document whether m_pullup guarantees to make the result
	   readable or preserves the read-only vs read/write aspect of
	   the input.

	3. Decide how to fix the callers that need contiguous results:

	   (a) No change, if m_makewritable is changed to make the
	       result contiguous.

	   (b) Call m_pullup _before_, if that's the right thing.

	   (c) Call m_pullup _after_, if that's the right thing.