On Mon, Jan 16, 2017 at 08:34:43PM +0100, Alexander Bluhm wrote:
> If I implement the same trick for newp, I can avoid the "netlock
> locking against myself" with sysctl on memory mapped files over
> NFS. Of course other copyin/copyout paths like pf(4) ioctl(2) still
> have to be checked. IPsec pfkey seem to use the sysctl mechanism.
Hrvoje Popovski has tested the diff and found some ugly
pmap_unwire: wiring for pmap 0xffffff00075f5210 va 0x7f7ffffd5000 didn't change!
kernel printfs. The happens when sysctl(8) writes a value.
If oldp and newp are in the same page, I have called uvm_vsunlock()
twice on the same address. I have added a simple check that does
not cover complicated overlappings but catches the common case.
Also I think PROT_READ for the newp should be enough.
Does anybody know, why the oldp is mapped PROT_READ | PROT_WRITE?
Is PROT_WRITE not sufficient?
bluhm
Index: kern/kern_sysctl.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.320
diff -u -p -r1.320 kern_sysctl.c
--- kern/kern_sysctl.c 11 Nov 2016 18:59:09 -0000 1.320
+++ kern/kern_sysctl.c 16 Jan 2017 22:37:42 -0000
@@ -157,7 +157,7 @@ sys_sysctl(struct proc *p, void *v, regi
syscallarg(void *) new;
syscallarg(size_t) newlen;
} */ *uap = v;
- int error, dolock = 1;
+ int error;
size_t savelen = 0, oldlen = 0;
sysctlfn *fn;
int name[CTL_MAXNAME];
@@ -219,30 +219,45 @@ sys_sysctl(struct proc *p, void *v, regi
if (SCARG(uap, oldlenp) &&
(error = copyin(SCARG(uap, oldlenp), &oldlen, sizeof(oldlen))))
return (error);
- if (SCARG(uap, old) != NULL) {
+ if (SCARG(uap, old) != NULL || SCARG(uap, new) != NULL) {
if ((error = rw_enter(&sysctl_lock, RW_WRITE|RW_INTR)) != 0)
return (error);
- if (dolock) {
- if (atop(oldlen) > uvmexp.wiredmax - uvmexp.wired) {
- rw_exit_write(&sysctl_lock);
- return (ENOMEM);
- }
- error = uvm_vslock(p, SCARG(uap, old), oldlen,
- PROT_READ | PROT_WRITE);
- if (error) {
- rw_exit_write(&sysctl_lock);
- return (error);
- }
+ }
+ if (SCARG(uap, old) != NULL) {
+ if (atop(oldlen) > uvmexp.wiredmax - uvmexp.wired) {
+ error = ENOMEM;
+ goto unlock;
}
+ error = uvm_vslock(p, SCARG(uap, old), oldlen,
+ PROT_READ | PROT_WRITE);
+ if (error)
+ goto unlock;
savelen = oldlen;
}
+ if (SCARG(uap, new) != NULL) {
+ if (atop(SCARG(uap, newlen)) > uvmexp.wiredmax - uvmexp.wired) {
+ error = ENOMEM;
+ goto unwire;
+ }
+ error = uvm_vslock(p, SCARG(uap, new), SCARG(uap, newlen),
+ PROT_READ);
+ if (error)
+ goto unwire;
+ }
error = (*fn)(&name[1], SCARG(uap, namelen) - 1, SCARG(uap, old),
&oldlen, SCARG(uap, new), SCARG(uap, newlen), p);
- if (SCARG(uap, old) != NULL) {
- if (dolock)
- uvm_vsunlock(p, SCARG(uap, old), savelen);
+ if (SCARG(uap, new) != NULL && (SCARG(uap, old) == NULL ||
+ (trunc_page((vaddr_t)SCARG(uap, new)) <
+ trunc_page((vaddr_t)SCARG(uap, old)) ||
+ (round_page((vaddr_t)SCARG(uap, new) + SCARG(uap, newlen)) >
+ round_page((vaddr_t)SCARG(uap, old) + savelen)))))
+ uvm_vsunlock(p, SCARG(uap, new), SCARG(uap, newlen));
+ unwire:
+ if (SCARG(uap, old) != NULL)
+ uvm_vsunlock(p, SCARG(uap, old), savelen);
+ unlock:
+ if (SCARG(uap, old) != NULL || SCARG(uap, new) != NULL)
rw_exit_write(&sysctl_lock);
- }
if (error)
return (error);
if (SCARG(uap, oldlenp))
