Re: [PATCH] Fix the race between the fget() and close()

[Al Viro]

On Sat, Aug 31, 2013 at 05:53:11AM +0000, Liu, Chuansheng wrote:

> I think I found one of possible race here(two processes P1 and P2):
> P1 has the the files_struct pointer FILES1, P2 has the files_struct pointer FILES2,
> 
> When P1 open file, the new struct file pointer SHARE_FILE will be installed into FILES1,
> and file refcount is 1;
> 
> And in P1, we can get P2's files_struct FILES2, and thru _fd_install(), we can add SHARE_FILE
> into P2's FILES2.
> 
> Then the same file pointer SHARE_FILE stayed in both P1 and P2's files_struct, and the panic case
> will happen:
> P1                                                            P2
> Open the SHARE_FILE
> Installed SHARE_FILE into P2's file_struct FILES2

... without bumping refcount on SHARE_FILE?  Then you really have a big
problem.  task_fd_install() call is preceded by grabbing a reference
to the file we are installing, though...  BTW, /* TODO: fput? */ after
that call is really bogus - the code doesn't call fput() there and it's
quite correct as is, since at that point the reference had gone into
descriptor table we'd been installing into and doesn't need to be dropped.

> Ioctl(SHARE_FILE)                                                When P2 exiting,
>  fget_light()
>    due to FILES1->refcount is 1,                                     put_files_struct will be called,
>    there will be no RCU and SHARE_FILE refcount increasing                will close all files including SHARE_FILE
> 
> But at this time, P1 is still operate SHARE_FILE without the refcount safety.
> 
> Then the panic will happen at vfs_ioctl() due to the SHARE_FILE has been freed.
> 
> Is it allowable that installing one file pointer into another FILES_STRUCT? Seems binder is doing the similar things.
> In fact, if in ioctl function, we can call fget() instead of fget_light(), this panic can be avoided.
> 
> Is it making sense?

No, it doesn't.  For one thing, any reference in any files_struct should
contribute 1 to refcount of struct file.  For another, you can modify
files_struct *ONLY* if you hold a reference to it.  binder, a misdesigned
piece of shit it is, does that only via proc->files, which is set in
binder_mmap() by grabbing a new reference to current->files of mmap(2)
caller.  It is safe to do (nobody can switch task's ->files to another
files_struct under it) and once that's done, there's a pinned reference
to that files_struct.  If, at the time of task_fd_install(), it happens
to be task->files_struct of some process, its refcount is going to be
at least 2, fdget() done by that other process will see that descriptor
table is shared and will bump the refcount of file being accessed.

The subtle part here is that mmap() does *NOT* use fdget() - the property
we are aiming for is that if at the time of fdget() descriptor table
hadn't been shared, no new references that could be used to modify it
will be acquired until the matching fdput().  So binder_mmap() can
legitimately grab a reference to the descriptor table of calling process.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/