On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote: > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote: > > On Wed, Feb 10 2021, Martin Pieuchot <m...@openbsd.org> wrote: > > > > [...] > > > > > Which fields is the new lock protecting? Why isn't the KERNEL_LOCK() > > > enough? > > > > When I mentioned this potential lack of locking to Marcus, I was > > mistaken. Some of the functions in video.c are called from syscalls > > that are marked NOLOCK. But now that I have added some printf > > debugging, I can see that the kernel lock is held in places where > > I didn't expect it to be held (videoioctl, videoread, videoclose...). > > So there's probably a layer of locking that I am missing. > > > > Marcus, sorry for my misleading diff. O:) > > > > *crawls back under his rock* > > Just because a syscall is marked NOLOCK does not mean that all of it will > run without the KERNEL_LOCK. For example read and write are marked NOLOCK > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors. > This is why video(4) still runs with the KERNEL_LOCK. > The NOLOCK in syscalls.master just tell the kernel to not grab the > KERNEL_LOCK right at the start of the syscall.
OK, that wasn't clear to me either! If Jeremie has some space left under his rock, I would join :-) That basically means we need no extra locking in video(4) for this implementation, right? For a test I replaced the rw_enter_write() with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent ioctl programs, and all went fine. I would come up with a revised diff then for the device owner part. Just doing some polishing together with Anton on that.
