cifs: guard against entries being deleted from open file list

Commit Message

Shirish Pargaonkar April 18, 2011, 1:44 p.m. UTC

From: Shirish Pargaonkar <[e-mail]>


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 <[e-mail]>
---
 fs/cifs/file.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Comments

Steve French April 18, 2011, 2:21 p.m. UTC | #1

Is there a reproduction scenario or bug report for this (or was it
noted by inspection)?

On Mon, Apr 18, 2011 at 8:44 AM,  <[e-mail]> wrote:
> From: Shirish Pargaonkar <[e-mail]>
>
>
> 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 <[e-mail]>
> ---
>  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())
> --
> 1.6.0.2
>
>

Shirish Pargaonkar April 18, 2011, 2:24 p.m. UTC | #2

On Mon, Apr 18, 2011 at 9:21 AM, Steve French <[e-mail]> wrote:
> Is there a reproduction scenario or bug report for this (or was it
> noted by inspection)?
>
> On Mon, Apr 18, 2011 at 8:44 AM,  <[e-mail]> wrote:
>> From: Shirish Pargaonkar <[e-mail]>
>>
>>
>> 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 <[e-mail]>
>> ---
>>  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())
>> --
>> 1.6.0.2
>>
>>
>
>
>
> --
> Thanks,
>
> Steve
>

Steve,

IBM internal bugzilla 71379.

I think a stressed CIFS/SMB server can make cifs client this way!
Perhaps harder to recreate strating 1.69 version of cifs
(which has Jeff's patches for a slow server).

Regards,

Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton April 18, 2011, 2:50 p.m. UTC | #3

On Mon, 18 Apr 2011 08:44:35 -0500
[e-mail] wrote:

> From: Shirish Pargaonkar <[e-mail]>
> 
> 
> 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 <[e-mail]>
> ---
>  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.

Shirish Pargaonkar April 19, 2011, 3:02 a.m. UTC | #4

On Mon, Apr 18, 2011 at 9:50 AM, Jeff Layton <[e-mail]> wrote:
> On Mon, 18 Apr 2011 08:44:35 -0500
> [e-mail] wrote:
>
>> From: Shirish Pargaonkar <[e-mail]>
>>
>>
>> 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 <[e-mail]>
>> ---
>>  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 <[e-mail]>
>

Jeff, I do not mind taking a stab at it.  Do you have suggestsion/ideas
about how to approach it?

Regards,

Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton April 19, 2011, 11:49 a.m. UTC | #5

On Mon, 18 Apr 2011 22:02:21 -0500
Shirish Pargaonkar <[e-mail]> wrote:

> On Mon, Apr 18, 2011 at 9:50 AM, Jeff Layton <[e-mail]> wrote:
> > On Mon, 18 Apr 2011 08:44:35 -0500
> > [e-mail] wrote:
> >
> >> From: Shirish Pargaonkar <[e-mail]>
> >>
> >>
> >> 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 <[e-mail]>
> >> ---
> >>  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 <[e-mail]>
> >
> 
> 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...

Patch

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())