| From 054aa8d439b9185d4f5eb9a90282d1ce74772969 Mon Sep 17 00:00:00 2001 |
| From: Linus Torvalds <torvalds@linux-foundation.org> |
| Date: Wed, 1 Dec 2021 10:06:14 -0800 |
| Subject: fget: check that the fd still exists after getting a ref to it |
| |
| From: Linus Torvalds <torvalds@linux-foundation.org> |
| |
| commit 054aa8d439b9185d4f5eb9a90282d1ce74772969 upstream. |
| |
| Jann Horn points out that there is another possible race wrt Unix domain |
| socket garbage collection, somewhat reminiscent of the one fixed in |
| commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK"). |
| |
| See the extended comment about the garbage collection requirements added |
| to unix_peek_fds() by that commit for details. |
| |
| The race comes from how we can locklessly look up a file descriptor just |
| as it is in the process of being closed, and with the right artificial |
| timing (Jann added a few strategic 'mdelay(500)' calls to do that), the |
| Unix domain socket garbage collector could see the reference count |
| decrement of the close() happen before fget() took its reference to the |
| file and the file was attached onto a new file descriptor. |
| |
| This is all (intentionally) correct on the 'struct file *' side, with |
| RCU lookups and lockless reference counting very much part of the |
| design. Getting that reference count out of order isn't a problem per |
| se. |
| |
| But the garbage collector can get confused by seeing this situation of |
| having seen a file not having any remaining external references and then |
| seeing it being attached to an fd. |
| |
| In commit cbcf01128d0a ("af_unix: fix garbage collect vs MSG_PEEK") the |
| fix was to serialize the file descriptor install with the garbage |
| collector by taking and releasing the unix_gc_lock. |
| |
| That's not really an option here, but since this all happens when we are |
| in the process of looking up a file descriptor, we can instead simply |
| just re-check that the file hasn't been closed in the meantime, and just |
| re-do the lookup if we raced with a concurrent close() of the same file |
| descriptor. |
| |
| Reported-and-tested-by: Jann Horn <jannh@google.com> |
| Acked-by: Miklos Szeredi <mszeredi@redhat.com> |
| Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| fs/file.c | 4 ++++ |
| 1 file changed, 4 insertions(+) |
| |
| --- a/fs/file.c |
| +++ b/fs/file.c |
| @@ -708,6 +708,10 @@ loop: |
| file = NULL; |
| else if (!get_file_rcu_many(file, refs)) |
| goto loop; |
| + else if (__fcheck_files(files, fd) != file) { |
| + fput_many(file, refs); |
| + goto loop; |
| + } |
| } |
| rcu_read_unlock(); |
| |