On Sun, Mar 26, 2017 at 05:00:12PM +0200, Mateusz Guzik wrote:
> The patch is somewhat incorrect, although from what I checked it happens
> to generate the expected outcome in terms of assembly (the global pointer
> read only once and then a local copy accessed several times). You either
> need a linux-like READ_ONCE macros which casts through a volatile pointer
> or mark the global as volatile to prevent the compiler from issuing
> multiple reads in the future.
Note that our OpenBSD kernel is still implemented with a big kernel
lock in most places. So multi processor thinking and locking is
not necessary. The execution order can only be interrupted by
hardware interrups or explicitly rescheduling with tsleep(9). Here
the sleep is hidden in copyin(), mallocarray(), sosend(), malloc(),
ktrgenio().
Interrupts are not relevant, they never change syslogf. As tsleep()
is a function call, it automatically acts as compiler barrier. So
volatile is not needed.
> Remaining ses sof syslogf are also super fishy:
> 1. logclose
> 2. logioctl -> LIOCSFD
> FRELE(syslogf, p);
A few months ago I would have said FRELE() does not sleep. I think
this is currently still the case as we do not grab the netlock for
socketpairs. But we did for a while.
As we are heading towards multi processor in kernel, I would suggest
this diff. It orders FREF() and FRELE() in a way that we only
operate on refcounted global variables. Although not necessary
with the current sleeping points, I think it results in more robust
code.
ok?
bluhm
Index: kern/subr_log.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
retrieving revision 1.49
diff -u -p -r1.49 subr_log.c
--- kern/subr_log.c 24 Mar 2017 16:42:38 -0000 1.49
+++ kern/subr_log.c 26 Mar 2017 19:37:05 -0000
@@ -172,10 +172,12 @@ logopen(dev_t dev, int flags, int mode,
int
logclose(dev_t dev, int flag, int mode, struct proc *p)
{
+ struct file *fp;
- if (syslogf)
- FRELE(syslogf, p);
+ fp = syslogf;
syslogf = NULL;
+ if (fp)
+ FRELE(fp, p);
log_open = 0;
logsoftc.sc_state = 0;
return (0);
@@ -355,11 +357,11 @@ logioctl(dev_t dev, u_long com, caddr_t
case LIOCSFD:
if ((error = suser(p, 0)) != 0)
return (error);
- if ((error = getsock(p, *(int *)data, &fp)) != 0)
+ fp = syslogf;
+ if ((error = getsock(p, *(int *)data, &syslogf)) != 0)
return (error);
- if (syslogf)
- FRELE(syslogf, p);
- syslogf = fp;
+ if (fp)
+ FRELE(fp, p);
break;
default:
Index: kern/uipc_syscalls.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.150
diff -u -p -r1.150 uipc_syscalls.c
--- kern/uipc_syscalls.c 14 Feb 2017 09:46:21 -0000 1.150
+++ kern/uipc_syscalls.c 26 Mar 2017 19:37:05 -0000
@@ -1146,8 +1146,8 @@ getsock(struct proc *p, int fdes, struct
return (EBADF);
if (fp->f_type != DTYPE_SOCKET)
return (ENOTSOCK);
- *fpp = fp;
FREF(fp);
+ *fpp = fp;
return (0);
}
