exec: Move unshare_files and guarantee files_struct.count is correct

* exec: Move unshare_files and guarantee files_struct.count is correct
@ 2020-08-17 22:03 Eric W. Biederman
  2020-08-17 22:04 ` [PATCH 01/17] exec: Move unshare_files to fix posix file locking during exec Eric W. Biederman
                   ` (16 more replies)
  0 siblings, 17 replies; 48+ messages in thread
From: Eric W. Biederman @ 2020-08-17 22:03 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-fsdevel, criu, bpf, Linus Torvalds, Alexander Viro,
	Christian Brauner, Oleg Nesterov, Cyrill Gorcunov, Jann Horn,
	Kees Cook, Daniel P. Berrangé,
	Jeff Layton, Miklos Szeredi, Matthew Wilcox, J. Bruce Fields,
	Matthew Wilcox, Trond Myklebust, Chris Wright,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh

A while ago it was reported that posix file locking goes wrong when a
multi-threaded process calls exec.  I looked into the history and this
is definitely a regression, that should be fixed if we can.

This set of changes cleanups of the code in exec so hopefully this code
will not regress again.  Then it adds helpers and fixes the users of
files_struct so the reference count is only incremented if COPY_FILES is
passed to clone.  Then it removes helpers (get_files_struct,
__install_fd, __alloc_fd, __close_fd) that are no longer needed and
if used would encourage code that increments the count of files_struct
somewhere besides in clone when COPY_FILES is passed.

In addition to fixing the bug in exec and simplifing the code this set
of changes by virtue of getting files_struct.count correct it optimizes
fdget.  With proc and other places not temporarily increasing the count
on files_struct __fget_light should succeed more often in being able to
return a struct file without touching it's reference count.

Fixing the count in files_struct was suggested by Oleg[1].

For those that are interested in the history of this issue I have
included as much of it as I could find in the first change.

 fs/coredump.c            |   5 +--
 fs/exec.c                |  26 ++++++-----
 fs/file.c                | 110 +++++++++++++++++++++--------------------------
 fs/open.c                |   2 +-
 fs/proc/fd.c             |  47 +++++++-------------
 include/linux/fdtable.h  |  13 ++----
 include/linux/syscalls.h |   6 +--
 kernel/bpf/syscall.c     |  20 ++-------
 kernel/bpf/task_iter.c   |  43 +++++-------------
 kernel/fork.c            |  12 +++---
 kernel/kcmp.c            |  20 ++-------
 11 files changed, 109 insertions(+), 195 deletions(-)

Eric W. Biederman (17):
      exec: Move unshare_files to fix posix file locking during exec
      exec: Simplify unshare_files
      exec: Remove reset_files_struct
      kcmp: In kcmp_epoll_target use fget_task
      bpf: In bpf_task_fd_query use fget_task
      file: Implement fcheck_task
      proc/fd: In tid_fd_mode use fcheck_task
      proc/fd: In proc_fd_link use fcheck_task
      file: Implement fnext_task
      proc/fd: In proc_readfd_common use fnext_task
      bpf/task_iter: In task_file_seq_get_next use fnext_task
      proc/fd: In fdinfo seq_show don't use get_files_struct
      file: Remove get_files_struct
      file: Merge __fd_install into fd_install
      file: In f_dupfd read RLIMIT_NOFILE once.
      file: Merge __alloc_fd into alloc_fd
      file: Rename __close_fd to close_fd and remove the files parameter

[1] https://lkml.kernel.org/r/20180915160423.GA31461@redhat.com
Reported-by: Jeff Layton <jlayton@redhat.com>
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Eric

^ permalink raw reply	[flat|nested] 48+ messages in thread