Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: viro@zeniv.linux.org.uk
Cc: ebiederm@xmission.com, berrange@redhat.com,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH v3 0/3] exec: fix passing of file locks across execve in multithreaded processes
Date: Fri, 14 Sep 2018 06:53:07 -0400
Message-ID: <20180914105310.6454-1-jlayton@kernel.org> (raw)

v2: fix displaced_files cleanup in __do_execve_file
v3: fix thread_count handling in unshare syscall
    add new release_files_struct helper

The main difference from the earlier set is some cleanup and fixes for
the thread_count handling in the files_struct. The original looked a
bit more ugly without the new helper function, and didn't get the
refcounting right in the unshare case, which was discovered by the
kernel test robot seeing the new WARN pop up during one test.

At this point, I'm mainly interested in whether this is the correct
approach to fix this at all. It is along the lines of what Eric B.
suggested, but I'm not sure whether it may break other cases that I
haven't considered.

-------------------------8<----------------------

A few months ago, Dan reported that when you call execve in process that
is multithreaded, any traditional POSIX locks are silently dropped.

The problem is that we end up unsharing the files_struct from the
process very early during exec, when it looks like it's shared between
tasks. Eventually, when the other, non-exec'ing tasks are killed, we
tear down the old files_struct. That ends up tearing down the old
files_struct, which ends up looking like a close() was issues on each
open fd and that causes the locks to be dropped.

This patchset is a second stab at fixing this issue, this time following
the method suggested by Eric Biederman. The idea here is to move the
unshare_files() call after de_thread(), which helps ensure that we only
unshare the files_struct when it's truly shared between different
processes, and not just when the exec'ing process is multithreaded.

This seems to fix the originally reported problem (now known as xfstest
generic/484), and basic testing doesn't seem to show any issues.

During the original discussion though, Al had mentioned that this could
be problematic due to the fdtable being modifiable by other threads
(or even processes) during the binfmt probe. That may make this idea
non-viable.

I'm also not terribly thrilled with the way this sprinkles the
files_struct->file_lock all over the place. It may be possible to do
some of this with atomic ops if the basic approach turns out to be
reasonable.

Comments and suggestions welcome.

Jeff Layton (3):
  exec: separate thread_count for files_struct
  exec: delay clone(CLONE_FILES) if task associated with current
    files_struct is exec'ing
  exec: do unshare_files after de_thread

 fs/coredump.c           |  2 +-
 fs/exec.c               | 27 +++++++++++++++++++--------
 fs/file.c               | 19 +++++++++++++++++--
 include/linux/binfmts.h |  1 +
 include/linux/fdtable.h |  3 +++
 kernel/fork.c           | 25 ++++++++++++++++++++-----
 6 files changed, 61 insertions(+), 16 deletions(-)

-- 
2.17.1

             reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-14 10:53 Jeff Layton [this message]
2018-09-14 10:53 ` [PATCH v3 1/3] exec: separate thread_count for files_struct Jeff Layton
2018-09-15 16:04   ` Oleg Nesterov
2018-09-16 16:10     ` Eric W. Biederman
2018-09-17 15:24       ` Oleg Nesterov
2018-09-17 20:45         ` Eric W. Biederman
2018-09-14 10:53 ` [PATCH v3 2/3] exec: delay clone(CLONE_FILES) if task associated with current files_struct is exec'ing Jeff Layton
2018-09-14 10:53 ` [PATCH v3 3/3] exec: do unshare_files after de_thread Jeff Layton
2018-09-15 16:37   ` Oleg Nesterov
2018-09-16 16:49     ` Eric W. Biederman
2018-09-17 15:28       ` Oleg Nesterov
2018-09-16 16:59   ` Eric W. Biederman
2018-09-16 17:38 ` [RFC][PATCH 0/3] exec: Moving unshare_files_struct Eric W. Biederman
2018-09-16 17:39   ` [RFC][PATCH 1/3] exec: Move unshare_files down to avoid locks being dropped on exec Eric W. Biederman
2018-09-17 15:49     ` Oleg Nesterov
2018-09-16 17:40   ` [RFC][PATCH 2/3] exec: Simplify unshare_files Eric W. Biederman
2018-09-17 16:23     ` Oleg Nesterov
2018-09-17 20:26       ` Eric W. Biederman
2018-09-16 17:41   ` [RFC][PATCH 3/3] exec: Remove reset_files_struct Eric W. Biederman
2018-09-17 15:59   ` [RFC][PATCH 0/3] exec: Moving unshare_files_struct Oleg Nesterov
2018-09-18 22:18     ` Eric W. Biederman
2018-09-17 16:24   ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180914105310.6454-1-jlayton@kernel.org \
    --to=jlayton@kernel.org \
    --cc=berrange@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git