Re: svn commit: r298656 - head/sys/kern

2016-04-26 Thread James Gritton
 

It turns out I was just about to commit that anyway, so the only
difference is I didn't need to put "(mis)" in the comment :-). ipcs(1)
was prone to allocation errors that I could bypass by rewriting the
sysctls without sbufs. 

So false positive, but for the best anyway. 

- Jamie 

On 2016-04-26 12:46, Conrad Meyer wrote: 

> The sbuf use here was fine before.
> 
> Best, Conrad 
> 
> On Tue, Apr 26, 2016 at 11:17 AM, Jamie Gritton  wrote:
> 
>> Author: jamie
>> Date: Tue Apr 26 18:17:44 2016
>> New Revision: 298656
>> URL: https://svnweb.freebsd.org/changeset/base/298656 [1]
>> 
>> Log:
>> Redo the changes to the SYSV IPC sysctl functions from r298585, so they
>> don't (mis)use sbufs.
>> 
>> PR: 48471
>> 
>> Modified:
>> head/sys/kern/sysv_msg.c
>> head/sys/kern/sysv_sem.c
>> head/sys/kern/sysv_shm.c
>> 
>> Modified: head/sys/kern/sysv_msg.c
>> ==
>> --- head/sys/kern/sysv_msg.c Tue Apr 26 18:11:45 2016 (r298655)
>> +++ head/sys/kern/sysv_msg.c Tue Apr 26 18:17:44 2016 (r298656)
>> @@ -65,7 +65,6 @@ __FBSDID("$FreeBSD$");
>> #include 
>> #include 
>> #include 
>> -#include 
>> #include 
>> #include 
>> #include 
>> @@ -1423,38 +1422,28 @@ sys_msgrcv(td, uap)
>> static int
>> sysctl_msqids(SYSCTL_HANDLER_ARGS)
>> {
>> - struct sbuf sb;
>> - struct msqid_kernel tmp, empty;
>> - struct msqid_kernel *msqkptr;
>> - struct prison *rpr;
>> + struct msqid_kernel tmsqk;
>> + struct prison *pr, *rpr;
>> int error, i;
>> 
>> - error = sysctl_wire_old_buffer(req, 0);
>> - if (error != 0)
>> - goto done;
>> + pr = req->td->td_ucred->cr_prison;
>> rpr = msg_find_prison(req->td->td_ucred);
>> - sbuf_new_for_sysctl(&sb, NULL, sizeof(struct msqid_kernel) *
>> - msginfo.msgmni, req);
>> -
>> - bzero(&empty, sizeof(empty));
>> + error = 0;
>> for (i = 0; i < msginfo.msgmni; i++) {
>> - msqkptr = &msqids[i];
>> - if (msqkptr->u.msg_qbytes == 0 || rpr == NULL ||
>> - msq_prison_cansee(rpr, msqkptr) != 0) {
>> - msqkptr = ∅
>> - } else if (req->td->td_ucred->cr_prison !=
>> - msqkptr->cred->cr_prison) {
>> - bcopy(msqkptr, &tmp, sizeof(tmp));
>> - msqkptr = &tmp;
>> - msqkptr->u.msg_perm.key = IPC_PRIVATE;
>> + mtx_lock(&msq_mtx);
>> + if (msqids[i].u.msg_qbytes == 0 || rpr == NULL ||
>> + msq_prison_cansee(rpr, &msqids[i]) != 0)
>> + bzero(&tmsqk, sizeof(tmsqk));
>> + else {
>> + tmsqk = msqids[i];
>> + if (tmsqk.cred->cr_prison != pr)
>> + tmsqk.u.msg_perm.key = IPC_PRIVATE;
>> }
>> -
>> - sbuf_bcat(&sb, msqkptr, sizeof(*msqkptr));
>> + mtx_unlock(&msq_mtx);
>> + error = SYSCTL_OUT(req, &tmsqk, sizeof(tmsqk));
>> + if (error != 0)
>> + break;
>> }
>> - error = sbuf_finish(&sb);
>> - sbuf_delete(&sb);
>> -
>> -done:
>> return (error);
>> }
>> 
>> @@ -1470,7 +1459,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, msgssz,
>> "Size of a message segment");
>> SYSCTL_INT(_kern_ipc, OID_AUTO, msgseg, CTLFLAG_RDTUN, &msginfo.msgseg, 0,
>> "Number of message segments");
>> -SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids, CTLTYPE_OPAQUE | CTLFLAG_RD,
>> +SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids,
>> + CTLTYPE_OPAQUE | CTLFLAG_RD | CTLFLAG_MPSAFE,
>> NULL, 0, sysctl_msqids, "", "Message queue IDs");
>> 
>> static int
>> 
>> Modified: head/sys/kern/sysv_sem.c
>> ==
>> --- head/sys/kern/sysv_sem.c Tue Apr 26 18:11:45 2016 (r298655)
>> +++ head/sys/kern/sysv_sem.c Tue Apr 26 18:17:44 2016 (r298656)
>> @@ -52,7 +52,6 @@ __FBSDID("$FreeBSD$");
>> #include 
>> #include 
>> #include 
>> -#include 
>> #include 
>> #include 
>> #include 
>> @@ -220,7 +219,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, semvmx,
>> "Semaphore maximum value");
>> SYSCTL_INT(_kern_ipc, OID_AUTO, semaem, CTLFLAG_RWTUN, &seminfo.semaem, 0,
>> "Adjust on exit max value");
>> -SYSCTL_PROC(_kern_ipc, OID_AUTO, sema, CTLTYPE_OPAQUE | CTLFLAG_RD,
>> +SYSCTL_PROC(_kern_ipc, OID_AUTO, sema,
>> + CTLTYPE_OPAQUE | CTLFLAG_RD | CTLFLAG_MPSAFE,
>> NULL, 0, sysctl_sema, "", "Semaphore id pool");
>> 
>> static struct syscall_helper_data sem_syscalls[] = {
>> @@ -1465,38 +1465,28 @@ semexit_myhook(void *arg, struct proc *p
>> static int
>> sysctl_sema(SYSCTL_HANDLER_ARGS)
>> {
>> - struct prison *rpr;
>> - struct sbuf sb;
>> - struct semid_kernel tmp, empty;
>> - struct semid_kernel *semakptr;
>> + struct prison *pr, *rpr;
>> + struct semid_kernel tsemak;
>> int error, i;
>> 
>> - error = sysctl_wire_old_buffer(req, 0);
>> - if (error != 0)
>> - goto done;
>> + pr = req->td->td_ucred->cr_prison;
>> rpr = sem_find_prison(req->td->td_ucred);
>> - sbuf_new_for_sysctl(&sb, NULL, sizeof(struct semid_kernel) *
>> - seminfo.semmni, req);
>> -
>> - bzero(&empty, sizeof(empty));
>> + error = 0;
>> for (i = 0; i < seminfo.semmni; i++) {
>> - semakptr = &sema[i];
>> - if ((semakptr->u.sem_perm.mode & SEM_ALLOC) == 0 ||
>> - rpr == NULL || sem_prison_cansee(rpr, semakptr) != 0) {
>> - semakptr = ∅
>> - } else if (req->td->td_ucred->cr_prison !=
>> - semakptr->cred->cr_pri

Re: svn commit: r298656 - head/sys/kern

2016-04-26 Thread Conrad Meyer
The sbuf use here was fine before.

Best,
Conrad

On Tue, Apr 26, 2016 at 11:17 AM, Jamie Gritton  wrote:

> Author: jamie
> Date: Tue Apr 26 18:17:44 2016
> New Revision: 298656
> URL: https://svnweb.freebsd.org/changeset/base/298656
>
> Log:
>   Redo the changes to the SYSV IPC sysctl functions from r298585, so they
>   don't (mis)use sbufs.
>
>   PR:   48471
>
> Modified:
>   head/sys/kern/sysv_msg.c
>   head/sys/kern/sysv_sem.c
>   head/sys/kern/sysv_shm.c
>
> Modified: head/sys/kern/sysv_msg.c
>
> ==
> --- head/sys/kern/sysv_msg.cTue Apr 26 18:11:45 2016(r298655)
> +++ head/sys/kern/sysv_msg.cTue Apr 26 18:17:44 2016(r298656)
> @@ -65,7 +65,6 @@ __FBSDID("$FreeBSD$");
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -1423,38 +1422,28 @@ sys_msgrcv(td, uap)
>  static int
>  sysctl_msqids(SYSCTL_HANDLER_ARGS)
>  {
> -   struct sbuf sb;
> -   struct msqid_kernel tmp, empty;
> -   struct msqid_kernel *msqkptr;
> -   struct prison *rpr;
> +   struct msqid_kernel tmsqk;
> +   struct prison *pr, *rpr;
> int error, i;
>
> -   error = sysctl_wire_old_buffer(req, 0);
> -   if (error != 0)
> -   goto done;
> +   pr = req->td->td_ucred->cr_prison;
> rpr = msg_find_prison(req->td->td_ucred);
> -   sbuf_new_for_sysctl(&sb, NULL, sizeof(struct msqid_kernel) *
> -   msginfo.msgmni, req);
> -
> -   bzero(&empty, sizeof(empty));
> +   error = 0;
> for (i = 0; i < msginfo.msgmni; i++) {
> -   msqkptr = &msqids[i];
> -   if (msqkptr->u.msg_qbytes == 0 || rpr == NULL ||
> -   msq_prison_cansee(rpr, msqkptr) != 0) {
> -   msqkptr = ∅
> -   } else if (req->td->td_ucred->cr_prison !=
> -   msqkptr->cred->cr_prison) {
> -   bcopy(msqkptr, &tmp, sizeof(tmp));
> -   msqkptr = &tmp;
> -   msqkptr->u.msg_perm.key = IPC_PRIVATE;
> +   mtx_lock(&msq_mtx);
> +   if (msqids[i].u.msg_qbytes == 0 || rpr == NULL ||
> +   msq_prison_cansee(rpr, &msqids[i]) != 0)
> +   bzero(&tmsqk, sizeof(tmsqk));
> +   else {
> +   tmsqk = msqids[i];
> +   if (tmsqk.cred->cr_prison != pr)
> +   tmsqk.u.msg_perm.key = IPC_PRIVATE;
> }
> -
> -   sbuf_bcat(&sb, msqkptr, sizeof(*msqkptr));
> +   mtx_unlock(&msq_mtx);
> +   error = SYSCTL_OUT(req, &tmsqk, sizeof(tmsqk));
> +   if (error != 0)
> +   break;
> }
> -   error = sbuf_finish(&sb);
> -   sbuf_delete(&sb);
> -
> -done:
> return (error);
>  }
>
> @@ -1470,7 +1459,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, msgssz,
>  "Size of a message segment");
>  SYSCTL_INT(_kern_ipc, OID_AUTO, msgseg, CTLFLAG_RDTUN, &msginfo.msgseg, 0,
>  "Number of message segments");
> -SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids, CTLTYPE_OPAQUE | CTLFLAG_RD,
> +SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids,
> +CTLTYPE_OPAQUE | CTLFLAG_RD | CTLFLAG_MPSAFE,
>  NULL, 0, sysctl_msqids, "", "Message queue IDs");
>
>  static int
>
> Modified: head/sys/kern/sysv_sem.c
>
> ==
> --- head/sys/kern/sysv_sem.cTue Apr 26 18:11:45 2016(r298655)
> +++ head/sys/kern/sysv_sem.cTue Apr 26 18:17:44 2016(r298656)
> @@ -52,7 +52,6 @@ __FBSDID("$FreeBSD$");
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -220,7 +219,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, semvmx,
>  "Semaphore maximum value");
>  SYSCTL_INT(_kern_ipc, OID_AUTO, semaem, CTLFLAG_RWTUN, &seminfo.semaem, 0,
>  "Adjust on exit max value");
> -SYSCTL_PROC(_kern_ipc, OID_AUTO, sema, CTLTYPE_OPAQUE | CTLFLAG_RD,
> +SYSCTL_PROC(_kern_ipc, OID_AUTO, sema,
> +CTLTYPE_OPAQUE | CTLFLAG_RD | CTLFLAG_MPSAFE,
>  NULL, 0, sysctl_sema, "", "Semaphore id pool");
>
>  static struct syscall_helper_data sem_syscalls[] = {
> @@ -1465,38 +1465,28 @@ semexit_myhook(void *arg, struct proc *p
>  static int
>  sysctl_sema(SYSCTL_HANDLER_ARGS)
>  {
> -   struct prison *rpr;
> -   struct sbuf sb;
> -   struct semid_kernel tmp, empty;
> -   struct semid_kernel *semakptr;
> +   struct prison *pr, *rpr;
> +   struct semid_kernel tsemak;
> int error, i;
>
> -   error = sysctl_wire_old_buffer(req, 0);
> -   if (error != 0)
> -   goto done;
> +   pr = req->td->td_ucred->cr_prison;
> rpr = sem_find_prison(req->td->td_ucred);
> -   sbuf_new_for_sysctl(&sb, NULL, sizeof(struct semid_kernel) *
> -   seminfo.semmni, req);
> -
> -   bzero(&empty, sizeof(empt

svn commit: r298656 - head/sys/kern

2016-04-26 Thread Jamie Gritton
Author: jamie
Date: Tue Apr 26 18:17:44 2016
New Revision: 298656
URL: https://svnweb.freebsd.org/changeset/base/298656

Log:
  Redo the changes to the SYSV IPC sysctl functions from r298585, so they
  don't (mis)use sbufs.
  
  PR:   48471

Modified:
  head/sys/kern/sysv_msg.c
  head/sys/kern/sysv_sem.c
  head/sys/kern/sysv_shm.c

Modified: head/sys/kern/sysv_msg.c
==
--- head/sys/kern/sysv_msg.cTue Apr 26 18:11:45 2016(r298655)
+++ head/sys/kern/sysv_msg.cTue Apr 26 18:17:44 2016(r298656)
@@ -65,7 +65,6 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -1423,38 +1422,28 @@ sys_msgrcv(td, uap)
 static int
 sysctl_msqids(SYSCTL_HANDLER_ARGS)
 {
-   struct sbuf sb;
-   struct msqid_kernel tmp, empty;
-   struct msqid_kernel *msqkptr;
-   struct prison *rpr;
+   struct msqid_kernel tmsqk;
+   struct prison *pr, *rpr;
int error, i;
 
-   error = sysctl_wire_old_buffer(req, 0);
-   if (error != 0)
-   goto done;
+   pr = req->td->td_ucred->cr_prison;
rpr = msg_find_prison(req->td->td_ucred);
-   sbuf_new_for_sysctl(&sb, NULL, sizeof(struct msqid_kernel) *
-   msginfo.msgmni, req);
-
-   bzero(&empty, sizeof(empty));
+   error = 0;
for (i = 0; i < msginfo.msgmni; i++) {
-   msqkptr = &msqids[i];
-   if (msqkptr->u.msg_qbytes == 0 || rpr == NULL ||
-   msq_prison_cansee(rpr, msqkptr) != 0) {
-   msqkptr = ∅
-   } else if (req->td->td_ucred->cr_prison !=
-   msqkptr->cred->cr_prison) {
-   bcopy(msqkptr, &tmp, sizeof(tmp));
-   msqkptr = &tmp;
-   msqkptr->u.msg_perm.key = IPC_PRIVATE;
+   mtx_lock(&msq_mtx);
+   if (msqids[i].u.msg_qbytes == 0 || rpr == NULL ||
+   msq_prison_cansee(rpr, &msqids[i]) != 0)
+   bzero(&tmsqk, sizeof(tmsqk));
+   else {
+   tmsqk = msqids[i];
+   if (tmsqk.cred->cr_prison != pr)
+   tmsqk.u.msg_perm.key = IPC_PRIVATE;
}
-
-   sbuf_bcat(&sb, msqkptr, sizeof(*msqkptr));
+   mtx_unlock(&msq_mtx);
+   error = SYSCTL_OUT(req, &tmsqk, sizeof(tmsqk));
+   if (error != 0)
+   break;
}
-   error = sbuf_finish(&sb);
-   sbuf_delete(&sb);
-
-done:
return (error);
 }
 
@@ -1470,7 +1459,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, msgssz, 
 "Size of a message segment");
 SYSCTL_INT(_kern_ipc, OID_AUTO, msgseg, CTLFLAG_RDTUN, &msginfo.msgseg, 0,
 "Number of message segments");
-SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids, CTLTYPE_OPAQUE | CTLFLAG_RD,
+SYSCTL_PROC(_kern_ipc, OID_AUTO, msqids,
+CTLTYPE_OPAQUE | CTLFLAG_RD | CTLFLAG_MPSAFE,
 NULL, 0, sysctl_msqids, "", "Message queue IDs");
 
 static int

Modified: head/sys/kern/sysv_sem.c
==
--- head/sys/kern/sysv_sem.cTue Apr 26 18:11:45 2016(r298655)
+++ head/sys/kern/sysv_sem.cTue Apr 26 18:17:44 2016(r298656)
@@ -52,7 +52,6 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -220,7 +219,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, semvmx, 
 "Semaphore maximum value");
 SYSCTL_INT(_kern_ipc, OID_AUTO, semaem, CTLFLAG_RWTUN, &seminfo.semaem, 0,
 "Adjust on exit max value");
-SYSCTL_PROC(_kern_ipc, OID_AUTO, sema, CTLTYPE_OPAQUE | CTLFLAG_RD,
+SYSCTL_PROC(_kern_ipc, OID_AUTO, sema,
+CTLTYPE_OPAQUE | CTLFLAG_RD | CTLFLAG_MPSAFE,
 NULL, 0, sysctl_sema, "", "Semaphore id pool");
 
 static struct syscall_helper_data sem_syscalls[] = {
@@ -1465,38 +1465,28 @@ semexit_myhook(void *arg, struct proc *p
 static int
 sysctl_sema(SYSCTL_HANDLER_ARGS)
 {
-   struct prison *rpr;
-   struct sbuf sb;
-   struct semid_kernel tmp, empty;
-   struct semid_kernel *semakptr;
+   struct prison *pr, *rpr;
+   struct semid_kernel tsemak;
int error, i;
 
-   error = sysctl_wire_old_buffer(req, 0);
-   if (error != 0)
-   goto done;
+   pr = req->td->td_ucred->cr_prison;
rpr = sem_find_prison(req->td->td_ucred);
-   sbuf_new_for_sysctl(&sb, NULL, sizeof(struct semid_kernel) *
-   seminfo.semmni, req);
-
-   bzero(&empty, sizeof(empty));
+   error = 0;
for (i = 0; i < seminfo.semmni; i++) {
-   semakptr = &sema[i];
-   if ((semakptr->u.sem_perm.mode & SEM_ALLOC) == 0 ||
-   rpr == NULL || sem_prison_cansee(rpr, semakptr) != 0) {
-   semakptr = ∅
-   } else if (req->td->td_ucred->cr_prison !=
-