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))

Reply via email to