Page MenuHomeFreeBSD

unix/dgram: add a specific send method - uipc_sosend_dgram()
ClosedPublic

Authored by glebius on May 23 2022, 9:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 5, 10:37 AM
Unknown Object (File)
Fri, Apr 5, 10:37 AM
Unknown Object (File)
Fri, Apr 5, 10:37 AM
Unknown Object (File)
Fri, Apr 5, 10:37 AM
Unknown Object (File)
Fri, Apr 5, 10:37 AM
Unknown Object (File)
Fri, Apr 5, 10:37 AM
Unknown Object (File)
Fri, Apr 5, 10:17 AM
Unknown Object (File)
Feb 14 2024, 11:08 PM
Subscribers

Details

Summary

This is first step towards splitting classic BSD socket
implementation into separate classes. The first to be
split is PF_UNIX/SOCK_DGRAM as it has most differencies
to SOCK_STREAM sockets and to PF_INET sockets.

Historically a protocol shall provide two methods for sendmsg(2):
pru_sosend and pru_send. The former is a generic send method,
e.g. sosend_generic() which would internally call the latter,
uipc_send() in our case. There is one important exception, though,
the sendfile(2) code will call pru_send directly. But sendfile
doesn't work on SOCK_DGRAM, so we can do the trick. We will create
socket class specific uipc_sosend_dgram() which will carry only
important bits from sosend_generic() and uipc_send().

Diff Detail

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

Event Timeline

Note: this patch is a step towards D35303.

sys/kern/uipc_usrreq.c
1202

Shouldn't we also call unp_internalize() in the case where m != NULL? I know that calls from userspace will pass a uio, but it seems possible that some kernel consumer may pass control messages.

1220

Seems like this should be a routine in mbuf.h.

glebius added inline comments.
sys/kern/uipc_usrreq.c
1202

I'm pretty sure there is no valid case for a kernel socket to send control.

1220

I would do it as soon as there is a second place with the same sequence.

sys/kern/uipc_usrreq.c
1202

Maybe, but then we're making an equivalency "uio != NULL if and only if the sender is userspace", which is fragile IMHO and wasn't present in the replaced code. Formally it's not a bug since nothing will trigger it today, but it feels strange.

glebius added inline comments.
sys/kern/uipc_usrreq.c
1202

I always seen this equivalency as a strong statement! uio is a structure to pass data between user and kernel, so if a kernel thread uses it, then it does something very hack-a-like and it is their responsibility to fully emulate user-like context, e.g. be able to sleep.

sys/kern/uipc_usrreq.c
1202

I don't really understand. KTLS is (almost) an example: it converts the uio to mbufs before passing data to the protocol send method, in order to insert frame headers. Is that hacky? Why can't such a thing be done one layer up above sosend? I can easily imagine something like the Linuxulator needing to do this to implement some special Linux interface.

I don't want to argue too much since this is probably not a real problem for now, but it also seems easy to fix, unless I'm missing something. We would just have a contract which says that anything passing a control message should be prepared to sleep, no matter whether data is passed with an mbuf chain or a uio.

1207

If you still plan to omit the unp_internalize() call here, I'd suggest making this an explicit panic not depending on INVARIANTS. It's cheap to check.

glebius added inline comments.
sys/kern/uipc_usrreq.c
1202

I don't really understand. KTLS is (almost) an example: it converts the uio to mbufs before passing data to the protocol send method, in order to insert frame headers. Is that hacky? Why can't such a thing be done one layer up above sosend? I can easily imagine something like the Linuxulator needing to do this to implement some special Linux interface.

I meant hacky is to send uio from a kernel thread. It isn't hacky to send mbufs from a userland sosend() with uio substituted to mbufs by some intermediate shim. However, until this happens to any unix/dgram consumer, I would prefer to keep invariant that this doesn't happen.

I don't want to argue too much since this is probably not a real problem for now, but it also seems easy to fix, unless I'm missing something. We would just have a contract which says that anything passing a control message should be prepared to sleep, no matter whether data is passed with an mbuf chain or a uio.

We already have this contract, since processing many of the control messages requires sleeping.

1207

Will do.

sys/kern/uipc_usrreq.c
1202

Ok.

1227

This is racy, the PCB isn't locked at this point.

glebius marked an inline comment as done.

Move "from" assignment down where it originally was, so that it is
accessed under PCB lock.

glebius added inline comments.
sys/kern/uipc_usrreq.c
1227

fixed

This revision is now accepted and ready to land.May 30 2022, 1:13 PM
glebius marked an inline comment as done.

Fix a bug, rebase.

This revision now requires review to proceed.May 31 2022, 4:36 PM
  • A legacy SOCKBUF_UNLOCK() that was missed and now found by syzcaller.
This revision was not accepted when it landed; it landed in state Needs Review.Jun 24 2022, 4:10 PM
This revision was automatically updated to reflect the committed changes.