Page MenuHomeFreeBSD

unix: Fix some bugs in the SOCK_STREAM receive path
ClosedPublic

Authored by markj on Thu, Aug 6, 4:49 PM.
Tags
None
Referenced Files
F170511599: D58695.id183803.diff
Sat, Sep 5, 4:19 AM
F170335336: D58695.id183803.diff
Fri, Sep 4, 8:45 AM
F170328485: D58695.id183561.diff
Fri, Sep 4, 8:04 AM
F170313506: D58695.id183561.diff
Fri, Sep 4, 6:50 AM
Unknown Object (File)
Fri, Sep 4, 1:32 AM
Unknown Object (File)
Thu, Sep 3, 11:31 AM
Unknown Object (File)
Thu, Sep 3, 6:58 AM
Unknown Object (File)
Thu, Sep 3, 2:16 AM
Subscribers

Details

Summary

The main problem is with the handling of errors from unp_externalize().
It turns out that this was quite broken, and unfortunately it's easy to
trigger such errors (e.g., by setting a low per-process fd limit with
setrlimit()).

In non-peek mode, uipc_soreceive_stream_or_seqpacket() cuts a bunch of
mbufs from the head of the socket buffer, to be consumed by userspace.
When unp_externalize() returns an error, we splice the removed mbuf
chain back onto the head of the socket buffer. This is expensive, but
that's ok since such errors are rare.

The problem is that this cutting is not correctly implemented: it does
not clear the "next" pointer for the last mbuf in the chain, so it
still points to the first mbuf still resident in the socket buffer.
This means that mc_init_m() creates a chain that still includes the rest
of the socket buffer, so splicing the chain back into the socket buffer
does not work properly.

Fix this: fully detach the control chain from the socket buffer so that
we can safely use mc_init_m(). Then, incrementally add data mbufs,
taking care to handle "part".

Fix some related bugs while here:

  • Don't swallow the error if unp_externalize() fails and there's nothing left in the socket buffer (i.e., control->m_next == NULL).
  • Roll back changes to the partially read mbuf.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Thu, Aug 6, 4:49 PM
glebius added inline comments.
sys/kern/uipc_usrreq.c
1463
1570

May I ask to bring back the original variable name? There was a plan that entire function will work on mchain mc, that's why this name in this small scope is what it is. To be fair I can't promise that this plan will be executed anytime soon.

This revision is now accepted and ready to land.Fri, Aug 7, 9:52 PM
This revision was automatically updated to reflect the committed changes.
markj marked 2 inline comments as done.