On Mon, 18 Apr 2011 22:02:21 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Mon, Apr 18, 2011 at 9:50 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Mon, 18 Apr 2011 08:44:35 -0500
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >>
> >> It is possible that a close can occur while a file is
> >> being reopened which can result in list entry deleted
> >> from the list and an oops.
> >> Use list_for_each_entry_safe instead.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> ---
> >> Âfs/cifs/file.c | Â Â5 +++--
> >> Â1 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> index faf5952..aa29167 100644
> >> --- a/fs/cifs/file.c
> >> +++ b/fs/cifs/file.c
> >> @@ -1056,7 +1056,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
> >> Âstruct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
> >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool fsuid_only)
> >> Â{
> >> - Â Â struct cifsFileInfo *open_file;
> >> + Â Â struct cifsFileInfo *open_file, *tmpf;
> >> Â Â Â struct cifs_sb_info *cifs_sb;
> >> Â Â Â bool any_available = false;
> >> Â Â Â int rc;
> >> @@ -1079,7 +1079,8 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
> >>
> >> Â Â Â spin_lock(&cifs_file_list_lock);
> >> Ârefind_writable:
> >> - Â Â list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
> >> + Â Â list_for_each_entry_safe(open_file, tmpf, &cifs_inode->openFileList,
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â flist) {
> >> Â Â Â Â Â Â Â if (!any_available && open_file->pid != current->tgid)
> >> Â Â Â Â Â Â Â Â Â Â Â continue;
> >> Â Â Â Â Â Â Â if (fsuid_only && open_file->uid != current_fsuid())
> >
> > I don't think this is a real fix for this problem, and may cause other
> > issues. list_for_each_entry_safe is only protects you when you are
> > iterating over a list (usually while holding a lock) and removing those
> > entries. The *_safe part makes it ok to remove the entry that you're
> > currently dealing with from the list.
> >
> > Now, that can happen if you find an entry, do a cifsFileInfo_get, drop
> > the spinlock and then calling cifsFileInfo_put and putting the last
> > reference afterward. Here's the problem though -- once you drop the
> > spinlock, there's nothing that says that the next entry in the list is
> > necessarily still valid once you move to that entry.
> >
> > What if the entry *after* the one that you're currently dealing with is
> > removed from the list while you're iterating over it? This patch will
> > cause an oops in that situation now when it didn't before. So, I think
> > this patch will probably just trade one set of problems for another.
> >
> > I think the real fix is overhauling how all of the reopen_file stuff
> > works, but fixing it looks anything but straightforward.
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxx>
> >
>
> Jeff, I do not mind taking a stab at it. Do you have suggestsion/ideas
> about how to approach it?
>
One idea might be to break up the openFileList into two lists. One for
valid handles and one for invalid ones. When you mark the file invalid,
move it to the other list.
When calling find_writable_file, walk the list of valid handles first,
and then walk the other list and start reclaiming filehandles and
moving them back to the openFileList if you don't find one on the first
pass.
Getting the locking right on all of that will be "fun", though.
While you're at it, it might also be a good time to get rid of some of
these global locks. The cifs_file_list_lock for instance could probably
stand to be more granular.
Some of the callers of find_writable_file could also be eliminated --
the ones in the setattr codepath, specifically by having them use
iattr->ia_file field.
It also wouldn't hurt to merge find_readable_file and
find_writable_file into a single function that takes a fmode_t argument.
Or, consider something more general -- a generic filehandle acquisition
layer that looks for open files that meet criteria that can be used by
any opener or current caller of find_*_file.
The possibilities for glory are endless here...
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
