Messages in this thread | | | Subject | Re: [RFC v10][PATCH 09/13] Restore open file descriprtors | From | Dave Hansen <> | Date | Mon, 01 Dec 2008 11:22:04 -0800 |
| |
On Fri, 2008-11-28 at 11:27 +0000, Al Viro wrote: > On Wed, Nov 26, 2008 at 08:04:40PM -0500, Oren Laadan wrote: > > +/** > > + * cr_attach_get_file - attach (and get) lonely file ptr to a file descriptor > > + * @file: lonely file pointer > > + */ > > +static int cr_attach_get_file(struct file *file) > > +{ > > + int fd = get_unused_fd_flags(0); > > + > > + if (fd >= 0) { > > + fsnotify_open(file->f_path.dentry); > > + fd_install(fd, file); > > + get_file(file); > > + } > > + return fd; > > +} > > What happens if another thread closes the descriptor in question between > fd_install() and get_file()?
You're just saying to flip the get_file() and fd_install()?
> > + fd = cr_attach_file(file); /* no need to cleanup 'file' below */ > > + if (fd < 0) { > > + filp_close(file, NULL); > > + ret = fd; > > + goto out; > > + } > > + > > + /* register new <objref, file> tuple in hash table */ > > + ret = cr_obj_add_ref(ctx, file, parent, CR_OBJ_FILE, 0); > > + if (ret < 0) > > + goto out; > > Who said that file still exists at that point?
Ahhh. We're depending on the 'struct file' reference that comes from the fd table. That's why there is supposedly "no need to cleanup 'file' below". But, some other thread can come along and close() the fd, which will __fput() our poor 'struct file' and will make it go away. Next time we go and pull it out of the hash table, we go boom.
As a quick fix, I think we can just take another get_file() here. But, as Al notes, there are some much larger issues that we face with the fd_table and multi-thread access. They haven't "mattered" to us so far because we assume everything is either single-threaded or frozen. Sounds like Al isn't comfortable with this being integrated until a much more detailed look has been taken.
> BTW, there are shitloads of races here - references to fd and struct file * > are mixed in a way that breaks *badly* if descriptor table is played with > by another thread.
One of the things about this that bothers me is that it shares too little with existing VFS code. It calls into a ton of existing stuff but doesn't refactor anything that is currently there. Surely there are some common bits somewhere in the VFS that could be consolidated here.
-- Dave
| |