Re: soo_ioctl() & socket lock

2017-07-18 Thread Alexander Bluhm
On Tue, Jul 18, 2017 at 08:38:56AM +0200, Martin Pieuchot wrote:
> Where soo_ioctl() modifies `so_state', `so_rcv' or `so_snd' it needs the
> socket lock.
> 
> More fields might need the lock in the future but for the moment I'm
> concentrating on fields accessed in the TCP input path.
> 
> ok?

OK bluhm@

> 
> Index: kern/sys_socket.c
> ===
> RCS file: /cvs/src/sys/kern/sys_socket.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 sys_socket.c
> --- kern/sys_socket.c 22 Feb 2017 10:20:21 -  1.30
> +++ kern/sys_socket.c 18 Jul 2017 06:26:15 -
> @@ -78,13 +78,16 @@ soo_ioctl(struct file *fp, u_long cmd, c
>   switch (cmd) {
>  
>   case FIONBIO:
> + s = solock(so);
>   if (*(int *)data)
>   so->so_state |= SS_NBIO;
>   else
>   so->so_state &= ~SS_NBIO;
> - return (0);
> + sounlock(s);
> + break;
>  
>   case FIOASYNC:
> + s = solock(so);
>   if (*(int *)data) {
>   so->so_state |= SS_ASYNC;
>   so->so_rcv.sb_flags |= SB_ASYNC;
> @@ -94,43 +97,47 @@ soo_ioctl(struct file *fp, u_long cmd, c
>   so->so_rcv.sb_flags &= ~SB_ASYNC;
>   so->so_snd.sb_flags &= ~SB_ASYNC;
>   }
> - return (0);
> + sounlock(s);
> + break;
>  
>   case FIONREAD:
>   *(int *)data = so->so_rcv.sb_datacc;
> - return (0);
> + break;
>  
>   case SIOCSPGRP:
>   so->so_pgid = *(int *)data;
>   so->so_siguid = p->p_ucred->cr_ruid;
>   so->so_sigeuid = p->p_ucred->cr_uid;
> - return (0);
> + break;
>  
>   case SIOCGPGRP:
>   *(int *)data = so->so_pgid;
> - return (0);
> + break;
>  
>   case SIOCATMARK:
>   *(int *)data = (so->so_state_RCVATMARK) != 0;
> - return (0);
> - }
> - /*
> -  * Interface/routing/protocol specific ioctls:
> -  * interface and routing ioctls should have a
> -  * different entry since a socket's unnecessary
> -  */
> - if (IOCGROUP(cmd) == 'i') {
> - NET_LOCK(s);
> - error = ifioctl(so, cmd, data, p);
> - NET_UNLOCK(s);
> - return (error);
> + break;
> +
> + default:
> + /*
> +  * Interface/routing/protocol specific ioctls:
> +  * interface and routing ioctls should have a
> +  * different entry since a socket's unnecessary
> +  */
> + if (IOCGROUP(cmd) == 'i') {
> + NET_LOCK(s);
> + error = ifioctl(so, cmd, data, p);
> + NET_UNLOCK(s);
> + return (error);
> + }
> + if (IOCGROUP(cmd) == 'r')
> + return (EOPNOTSUPP);
> + s = solock(so);
> + error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL,
> + (struct mbuf *)cmd, (struct mbuf *)data, NULL, p));
> + sounlock(s);
> + break;
>   }
> - if (IOCGROUP(cmd) == 'r')
> - return (EOPNOTSUPP);
> - s = solock(so);
> - error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL, 
> - (struct mbuf *)cmd, (struct mbuf *)data, (struct mbuf *)NULL, p));
> - sounlock(s);
>  
>   return (error);
>  }



soo_ioctl() & socket lock

2017-07-18 Thread Martin Pieuchot
Where soo_ioctl() modifies `so_state', `so_rcv' or `so_snd' it needs the
socket lock.

More fields might need the lock in the future but for the moment I'm
concentrating on fields accessed in the TCP input path.

ok?

Index: kern/sys_socket.c
===
RCS file: /cvs/src/sys/kern/sys_socket.c,v
retrieving revision 1.30
diff -u -p -r1.30 sys_socket.c
--- kern/sys_socket.c   22 Feb 2017 10:20:21 -  1.30
+++ kern/sys_socket.c   18 Jul 2017 06:26:15 -
@@ -78,13 +78,16 @@ soo_ioctl(struct file *fp, u_long cmd, c
switch (cmd) {
 
case FIONBIO:
+   s = solock(so);
if (*(int *)data)
so->so_state |= SS_NBIO;
else
so->so_state &= ~SS_NBIO;
-   return (0);
+   sounlock(s);
+   break;
 
case FIOASYNC:
+   s = solock(so);
if (*(int *)data) {
so->so_state |= SS_ASYNC;
so->so_rcv.sb_flags |= SB_ASYNC;
@@ -94,43 +97,47 @@ soo_ioctl(struct file *fp, u_long cmd, c
so->so_rcv.sb_flags &= ~SB_ASYNC;
so->so_snd.sb_flags &= ~SB_ASYNC;
}
-   return (0);
+   sounlock(s);
+   break;
 
case FIONREAD:
*(int *)data = so->so_rcv.sb_datacc;
-   return (0);
+   break;
 
case SIOCSPGRP:
so->so_pgid = *(int *)data;
so->so_siguid = p->p_ucred->cr_ruid;
so->so_sigeuid = p->p_ucred->cr_uid;
-   return (0);
+   break;
 
case SIOCGPGRP:
*(int *)data = so->so_pgid;
-   return (0);
+   break;
 
case SIOCATMARK:
*(int *)data = (so->so_state_RCVATMARK) != 0;
-   return (0);
-   }
-   /*
-* Interface/routing/protocol specific ioctls:
-* interface and routing ioctls should have a
-* different entry since a socket's unnecessary
-*/
-   if (IOCGROUP(cmd) == 'i') {
-   NET_LOCK(s);
-   error = ifioctl(so, cmd, data, p);
-   NET_UNLOCK(s);
-   return (error);
+   break;
+
+   default:
+   /*
+* Interface/routing/protocol specific ioctls:
+* interface and routing ioctls should have a
+* different entry since a socket's unnecessary
+*/
+   if (IOCGROUP(cmd) == 'i') {
+   NET_LOCK(s);
+   error = ifioctl(so, cmd, data, p);
+   NET_UNLOCK(s);
+   return (error);
+   }
+   if (IOCGROUP(cmd) == 'r')
+   return (EOPNOTSUPP);
+   s = solock(so);
+   error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL,
+   (struct mbuf *)cmd, (struct mbuf *)data, NULL, p));
+   sounlock(s);
+   break;
}
-   if (IOCGROUP(cmd) == 'r')
-   return (EOPNOTSUPP);
-   s = solock(so);
-   error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL, 
-   (struct mbuf *)cmd, (struct mbuf *)data, (struct mbuf *)NULL, p));
-   sounlock(s);
 
return (error);
 }