I want to move a little bit forward with fine grained locking in UNIX
domain sockets area. My goal is to introduce the new rwlock(9) for
garbage collector data and keep existing `unp_lock' only for per-socket
data. This will allow to replace it by per-socket `so_lock' in the
future.
I like to start from unp_externalize(). It touches only garbage
collector data such as `unp_rights', `unp_msgcount' directly and
`unp_deferred' through unp_discard(). So `unp_lock' could be dropped
outside unp_externalize(). I don't want to introduce the new gc related
rwlock(9) with this diff, so `unp_lock' is still taken around garbage
collector data access. But right now this diff removes M_WAITOK
allocation from rwlock(9) and useless and weird fdplock() dances and
memory allocation from the error path. Also with this diff the only
place where we have `unp_lock' and fdplock() are both taken will be the
unp_internalize(). This allows to change their existing lock order or
completely avoid the case where these locks are both taken. This should
help to mpi@'s knote(9) work.
I want to do the unp_internalize() related work and introduce the new
garbage collector related rwlock(9) in the further diffs.
The socket locking could be not clean to somebody, so I like to put a
little explanation here. The solock() (`unp_lock') release is absolutely
safe around unp_externalize() within soreceive() path. The socket's
`so_rcv' buffer is still locked by sblock(). Concurrent thread could
disconnect this socket, but our soreceive() thread will be finished fine.
Such races are application specific. Also the lock sequence:
s = solock(so);
sblock(so, &so->so_rcv, ...);
sounlock(so, s);
s = solock(so);
does not provide lock order issue because the rwsleep_nsec(9) called
within sblock() releases passed rwlock(9). Since sblock() and sosleep()
release rwlock(9) taken by preceding solock(), the only `so_state' flags
and the PCB flags like `unp_flags' make socket consistent but not
solock().
Index: sys/kern/uipc_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.264
diff -u -p -r1.264 uipc_socket.c
--- sys/kern/uipc_socket.c 26 Jul 2021 05:51:13 -0000 1.264
+++ sys/kern/uipc_socket.c 2 Oct 2021 23:07:04 -0000
@@ -892,9 +892,11 @@ dontblock:
sbsync(&so->so_rcv, nextrecord);
if (controlp) {
if (pr->pr_domain->dom_externalize) {
+ sounlock(so, s);
error =
(*pr->pr_domain->dom_externalize)
(cm, controllen, flags);
+ s = solock(so);
}
*controlp = cm;
} else {
Index: sys/kern/uipc_usrreq.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.148
diff -u -p -r1.148 uipc_usrreq.c
--- sys/kern/uipc_usrreq.c 25 May 2021 22:45:09 -0000 1.148
+++ sys/kern/uipc_usrreq.c 2 Oct 2021 23:07:04 -0000
@@ -817,8 +817,6 @@ unp_externalize(struct mbuf *rights, soc
struct file *fp;
int nfds, error = 0;
- rw_assert_wrlock(&unp_lock);
-
/*
* This code only works because SCM_RIGHTS is the only supported
* control message type on unix sockets. Enforce this here.
@@ -834,7 +832,7 @@ unp_externalize(struct mbuf *rights, soc
controllen -= CMSG_ALIGN(sizeof(struct cmsghdr));
if (nfds > controllen / sizeof(int)) {
error = EMSGSIZE;
- goto restart;
+ goto out;
}
/* Make sure the recipient should be able to see the descriptors.. */
@@ -868,18 +866,13 @@ unp_externalize(struct mbuf *rights, soc
KERNEL_UNLOCK();
+ if (error)
+ goto out;
+
fds = mallocarray(nfds, sizeof(int), M_TEMP, M_WAITOK);
-restart:
fdplock(fdp);
- if (error != 0) {
- if (nfds > 0) {
- rp = ((struct fdpass *)CMSG_DATA(cm));
- unp_discard(rp, nfds);
- }
- goto out;
- }
-
+restart:
/*
* First loop -- allocate file descriptor table slots for the
* new descriptors.
@@ -895,17 +888,19 @@ restart:
if (error == ENOSPC) {
fdexpand(p);
- error = 0;
- } else {
- /*
- * This is the error that has historically
- * been returned, and some callers may
- * expect it.
- */
- error = EMSGSIZE;
+ goto restart;
}
+
fdpunlock(fdp);
- goto restart;
+
+ /*
+ * This is the error that has historically
+ * been returned, and some callers may
+ * expect it.
+ */
+
+ error = EMSGSIZE;
+ goto out;
}
/*
@@ -924,12 +919,15 @@ restart:
rp++;
}
+ fdpunlock(fdp);
/*
* Now that adding them has succeeded, update all of the
* descriptor passing state.
*/
rp = (struct fdpass *)CMSG_DATA(cm);
+
+ rw_enter_write(&unp_lock);
for (i = 0; i < nfds; i++) {
struct unpcb *unp;
@@ -939,6 +937,7 @@ restart:
unp->unp_msgcount--;
unp_rights--;
}
+ rw_exit_write(&unp_lock);
/*
* Copy temporary array to message and adjust length, in case of
@@ -947,10 +946,19 @@ restart:
memcpy(CMSG_DATA(cm), fds, nfds * sizeof(int));
cm->cmsg_len = CMSG_LEN(nfds * sizeof(int));
rights->m_len = CMSG_LEN(nfds * sizeof(int));
- out:
- fdpunlock(fdp);
+out:
if (fds != NULL)
free(fds, M_TEMP, nfds * sizeof(int));
+
+ if (error) {
+ if (nfds > 0) {
+ rp = ((struct fdpass *)CMSG_DATA(cm));
+ rw_enter_write(&unp_lock);
+ unp_discard(rp, nfds);
+ rw_exit_write(&unp_lock);
+ }
+ }
+
return (error);
}
