On Sun, Mar 26, 2017 at 10:04 PM, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
> 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. > > The previous patch replaced multiple reads of the global var with just one read and had the result stored in a local variable, which then is read multiple times. Even though the compiler ended up emitting one read of the global, it perfectly legal to emit several, thus the bug can still reappear. This can be prevented as described above. > > 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. With aforementioned caveat ignored, the patch indeed fixes the problem. I skimmed through and found few *sleep calls, but perhaps they cannot end up being executed for the type of socket accepted here. -- Mateusz Guzik <mjguzik gmail.com>
