> Date: Mon, 18 Jun 2018 09:07:58 +0200 > From: Martin Pieuchot <m...@openbsd.org> > > On 16/06/18(Sat) 19:28, Mark Kettenis wrote: > > > Date: Thu, 14 Jun 2018 14:28:07 +0200 > > > From: Martin Pieuchot <m...@openbsd.org> > > > > > > This version should fixes the previous issue where LARVAL files were > > > incorrectly accounted in find_last_set() resulting in a panic() in > > > fd_unused(). > > > > > > If you haven't read the previous explanations, the idea of this diff > > > is to publish file descriptors only when they are completely setup. > > > This will allow us to serialize access to file descriptors in a mp-safe > > > way, which is the last requirement for unlock most of the socket-related > > > syscalls. > > > > > > The important point of this diff is that we don't store on the descriptor > > > itself the fact that it isn't ready yet (LARVAL). Otherwise we have a > > > chicken & egg problem for reference counting. As pointed by Mathieu > > > Masson other BSDs do that by using a new structure for open files. We > > > might want to do that later when we'll turn these functions mp-safe :) > > > For now, using the existing bitmask is good enough. > > > > > > Here are previous explanations: > > > https://marc.info/?l=openbsd-tech&m=152579630904328&w=2 > > > https://marc.info/?l=openbsd-tech&m=152750590114899&w=2 > > > > > > Please test & report as usual. > > > > > > Ok? > > > > Sorry, but no, I don't think this diff is ok. Your new code inserts > > the struct file into the linked list of all open files on fdinsert(). > > Yes, that's the goal of my diff. No descriptor should be reachable by > another thread before fdinsert() has been called. Since the linked list > offers such possibility it should be delayed.
But the linked list doesn't point at file descriptors. It points at open files. Those are different concepts. An open file can have any number of file descriptors associated with it. And that number can even be zero. This happens when you pass a file descrptor to another proecess using sendmsg(2) and close it immediately. Until the other side calls recmsg(2) there is no file descriptor for the file, but it is still open. The sole purpose of the linked list is to implement the KERN_FILE_BYFILE sysctl. And all that sysctl does is copy the contents of the struct file to userland. The linked list should not be used for any other purpose. Userland has to deal with the fact that the contents of that struct file are stale already. I guess this point is a bit academic. The current file descriptor APIs are heavily geared towards opening a file and creating a file descriptor in one operation. I think that is somewhat of a design mistake. And it is not your design mistake. But it seems to be influencing your locking design and I'm not sure the direction you're heading is the right one. The diff you committed is mostly fine. I'll comment on the next diff to point out some alternatives instead. > > However, there is code that bails out and calls closef() before that > > fdinsert() happens. For exacmple in kern_exec.c:sys_execve() and in > > sys_pipe.c:dopipe() (if the 2nd falloc() call fails). > > There's no problem with that. Ah, you explicitly check the FIF_INSERTED flag. So you skip te LIST_REMOVE() if the struct file is not on the list. > > I really think falloc() should continue to insert the struct file in > > the list. That list is only used for userland tools like fstat and > > fuser. And those tools should handle open files that don't have a > > file descriptor. > > Such files almost never exist Mark, only in accept(2). All other sycalls > block on the fdplock(). I'm not going to change the sysctl interface > to make it MP-safe and fix all the related bug in all subsystems ;) > > However I'd glad if you could come up with a way to show such files in > a MP-safe way after I've committed my diff. You can even try visa@'s > diff: https://marc.info/?l=openbsd-tech&m=152794586504816&w=2 >
