svn commit: r300764 - head/sys/netinet

[John Baldwin]

On Friday, May 27, 2016 12:14:28 AM Bjoern A. Zeeb wrote:
> 
> > On 26 May 2016, at 18:35 , John Baldwin <jhb at FreeBSD.org> wrote:
> > 
> > Author: jhb
> > Date: Thu May 26 18:35:37 2016
> > New Revision: 300764
> > URL: https://svnweb.freebsd.org/changeset/base/300764
> > 
> > Log:
> >  Don't reuse the source mbuf in tcp_respond() if it is not writable.
> > 
> >  Not all mbufs passed up from device drivers are M_WRITABLE().  In
> >  particular, the Chelsio T4/T5 driver uses a feature called "buffer packing"
> >  to receive multiple frames in a single receive buffer.  The mbufs for
> >  these frames all share the same external storage so are treated as
> >  read-only by the rest of the stack when multiple frames are in flight.
> >  Previously tcp_respond() would blindly overwrite read-only mbufs when
> >  INVARIANTS was disabled or panic with an assertion failure if INVARIANTS
> >  was enabled.  Note that the new case is a bit of a mix of the two other
> >  cases in tcp_respond().  The TCP and IP headers must be copied explicitly
> >  into the new mbuf instead of being inherited (similar to the m == NULL
> >  case), but the addresses and ports must be swapped in the reply (similar
> >  to the m != NULL case).
> 
> Is the same true for ICMP(v6) replies?

Hmm, icmp_error() always allocates a new packet, but icmp_respond() does not.
There's no assertion to trip in that case. :-/

However, even if this change really isn't quite "perfect".  Specifically,
with "buffer packing" the multiple mbufs sharing a single backing buffer via
m_ext are all using non-overlapping regions of that buffer, and each mbuf does
have exclusive access to its portion of the buffer.  You just can't grow it.
So, if a reuse does not extend the buffer for these particular mbufs then it
is actually ok.  (So icmp_respond() is actually ok, and tcp_respond() is also
probably ok in practice.)  However, we don't have any way to distinguish this
use case from multiple mbufs sharing possibly-overlapping regions of a
buffer.

Fixing icmp_respond() is probably simpler as you can just m_dup() if the source
is not writable and then m_freem() the original and reuse the clone as 'm' for
the rest of the function.

-- 
John Baldwin