Hi,
The following diff sets NOLOCK on dup(2) and dup2(2) syscalls. It is a
part of "unlock more syscalls".
All the hard work was already done by mpi@. My changes are mostly to add
few KASSERT() or comments in current code.
The key function for pipe creation is dopipe(). It creates 2 pipes
struct using pipecreate(), allocate descriptors and insert them (this
part is already KERNEL_LOCK safe).
The specific part are pipe_create() and error code which call pipeclose().
1. pipe_create()
pipe_create() do only initialization with safe code, or already take the
KERNEL_LOCK like for km_alloc() or km_free().
It also call pipespace() to initialize the buffer. I added some
KASSERT() inside it to make visible the expectation: the pipe should
be uninitialized (like when called from pipe_create() or locked (with
PIPE_LOCK, as the buffer will be freed and realloced).
While here, I also added a KASSERT() to stand the buffer is empty before
replacing it.
2. pipeclose()
In the general case, pipeclose() isn't safe to be called without KERNEL_LOCK:
- if the pipe is busy, it could call wakeup() and tsleep()
- if the pipe has peer, it could call pipeselwakeup() and wakeup()
But we are in dopipe(), so it is a special case:
- the pipe isn't used for now, so pipe_busy couldn't be set
- the error code paths possibility calling pipeclose() are before setting
pipe_peer, so the pipe has not peer when called.
So I added a comment to say it is safe to call pipeclose() here.
And a side question: what is the purpose of `amountpipekva' variable ?
it is incremented/decremented to reflect the total size of all pipe
buffers in use, but it is used nowhere outside pipespace() for inc
and pipe_free_kmem() for dec. should be deleted or it is useful for
debugging (via ddb perhaps) ?
For testing, please regenerate syscalls after applying the diff, and
before compiling the kernel:
$ cd /sys/kern && make syscalls
Comments or OK ?
--
Sebastien Marie
Index: kern/syscalls.master
===================================================================
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.192
diff -u -p -r1.192 syscalls.master
--- kern/syscalls.master 22 Jun 2019 06:49:14 -0000 1.192
+++ kern/syscalls.master 24 Jun 2019 10:26:14 -0000
@@ -216,7 +216,7 @@
socklen_t namelen); }
99 STD { int sys_getdents(int fd, void *buf, size_t buflen); }
100 STD { int sys_getpriority(int which, id_t who); }
-101 STD { int sys_pipe2(int *fdp, int flags); }
+101 STD NOLOCK { int sys_pipe2(int *fdp, int flags); }
102 STD { int sys_dup3(int from, int to, int flags); }
103 STD { int sys_sigreturn(struct sigcontext *sigcntxp); }
104 STD { int sys_bind(int s, const struct sockaddr *name, \
@@ -448,7 +448,7 @@
260 UNIMPL
261 UNIMPL
262 UNIMPL
-263 STD { int sys_pipe(int *fdp); }
+263 STD NOLOCK { int sys_pipe(int *fdp); }
264 STD { int sys_fhopen(const fhandle_t *fhp, int flags); }
265 UNIMPL
266 UNIMPL
Index: kern/sys_pipe.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.88
diff -u -p -r1.88 sys_pipe.c
--- kern/sys_pipe.c 22 Jun 2019 06:48:25 -0000 1.88
+++ kern/sys_pipe.c 24 Jun 2019 10:30:12 -0000
@@ -198,6 +198,7 @@ free3:
free2:
fdpunlock(fdp);
free1:
+ /* fine without KERNEL_LOCK because just created */
pipeclose(wpipe);
pipeclose(rpipe);
return (error);
@@ -214,12 +215,18 @@ pipespace(struct pipe *cpipe, u_int size
{
caddr_t buffer;
+ /* pipe should be uninitialized or locked */
+ KASSERT((cpipe->pipe_buffer.buffer == NULL) ||
+ (cpipe->pipe_state & PIPE_LOCK));
+
+ /* buffer should be empty */
+ KASSERT(cpipe->pipe_buffer.cnt == 0);
+
KERNEL_LOCK();
buffer = km_alloc(size, &kv_any, &kp_pageable, &kd_waitok);
KERNEL_UNLOCK();
- if (buffer == NULL) {
+ if (buffer == NULL)
return (ENOMEM);
- }
/* free old resources if we are resizing */
pipe_free_kmem(cpipe);
@@ -227,7 +234,6 @@ pipespace(struct pipe *cpipe, u_int size
cpipe->pipe_buffer.size = size;
cpipe->pipe_buffer.in = 0;
cpipe->pipe_buffer.out = 0;
- cpipe->pipe_buffer.cnt = 0;
atomic_add_int(&amountpipekva, cpipe->pipe_buffer.size);
@@ -244,6 +250,8 @@ pipe_create(struct pipe *cpipe)
/* so pipe_free_kmem() doesn't follow junk pointer */
cpipe->pipe_buffer.buffer = NULL;
+ cpipe->pipe_buffer.cnt = 0;
+
/*
* protect so pipeclose() doesn't follow a junk pointer
* if pipespace() fails.
@@ -303,6 +311,7 @@ pipeselwakeup(struct pipe *cpipe)
selwakeup(&cpipe->pipe_sel);
} else
KNOTE(&cpipe->pipe_sel.si_note, 0);
+
if (cpipe->pipe_state & PIPE_ASYNC)
pgsigio(&cpipe->pipe_sigio, SIGIO, 0);
}
