From patchwork Wed Mar 7 18:43:28 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 10264673 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 96002602C8 for ; Wed, 7 Mar 2018 18:43:33 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7A1AD295F1 for ; Wed, 7 Mar 2018 18:43:33 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6E8E2295F6; Wed, 7 Mar 2018 18:43:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DEAEC295F1 for ; Wed, 7 Mar 2018 18:43:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753949AbeCGSnb (ORCPT ); Wed, 7 Mar 2018 13:43:31 -0500 Received: from mail.kernel.org ([198.145.29.99]:39114 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753901AbeCGSna (ORCPT ); Wed, 7 Mar 2018 13:43:30 -0500 Received: from tleilax.poochiereds.net (cpe-71-70-156-158.nc.res.rr.com [71.70.156.158]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B626C2133D; Wed, 7 Mar 2018 18:43:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B626C2133D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=jlayton@kernel.org From: Jeff Layton To: linux-fsdevel@vger.kernel.org Cc: berrange@redhat.com, dhowells@redhat.com, bfields@fieldses.org Subject: [PATCH] locks: change POSIX lock ownership on execve when file_struct is displaced Date: Wed, 7 Mar 2018 13:43:28 -0500 Message-Id: <20180307184328.9333-1-jlayton@kernel.org> X-Mailer: git-send-email 2.14.3 MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Jeff Layton POSIX mandates that open fds and their associated file locks should be preserved across an execve. This works, unless the process is multithreaded at the time that execve is called. In that case, we'll end up unsharing the files_struct but the locks will still have their fl_owner pointed at the old one. Eventually the last reference to the old files_struct is put and the locks get torn down. The result is that all of your files will be intact with none of the locks you held before execve. The simple answer to this is "use OFD locks", but this is a nasty surprise and violates the spec. On a successful execve, change ownership of any file_locks associated with the old files_struct to the new one, if we ended up swapping it out. Reported-by: Daniel P. Berrangé Signed-off-by: Jeff Layton --- fs/exec.c | 4 +++- fs/file.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ fs/locks.c | 43 +++++++++++++++++++++++++++++++++++++++++++ include/linux/fdtable.h | 1 + include/linux/fs.h | 1 + 5 files changed, 92 insertions(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index 7eb8d21bcab9..e9f154f9bad9 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1812,8 +1812,10 @@ static int do_execveat_common(int fd, struct filename *filename, free_bprm(bprm); kfree(pathbuf); putname(filename); - if (displaced) + if (displaced) { + change_lock_owners(current->files, displaced); put_files_struct(displaced); + } return retval; out: diff --git a/fs/file.c b/fs/file.c index 42f0db4bd0fb..cbffcdeb22bb 100644 --- a/fs/file.c +++ b/fs/file.c @@ -365,6 +365,50 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp) return NULL; } +/** + * change_lock_owners - change lock ownership from old files struct to new + * @files: new files struct to own locks + * @old: old files struct that previously held locks + * + * On execve, a process may end up with a new files_struct. In that case, we + * must change all of the locks that were owned by the previous files_struct + * to the new one. + */ +void change_lock_owners(struct files_struct *new, struct files_struct *old) +{ + struct fdtable *fdt = rcu_dereference_raw(new->fdt); + unsigned int i, j = 0; + + /* + * It is safe to dereference the fd table without RCU or ->file_lock + * because this is the only reference to the files structure. If that's + * ever not the case, just warn and don't change anything. + */ + if (unlikely(atomic_read(&new->count) != 1)) { + WARN_ON_ONCE(1); + return; + } + + for (;;) { + unsigned long set; + i = j * BITS_PER_LONG; + if (i >= fdt->max_fds) + break; + set = fdt->open_fds[j++]; + while (set) { + if (set & 1) { + struct file * file = fdt->fd[i]; + if (file) { + posix_chown_locks(file, old, new); + cond_resched(); + } + } + i++; + set >>= 1; + } + } +} + static struct fdtable *close_files(struct files_struct * files) { /* diff --git a/fs/locks.c b/fs/locks.c index d6ff4beb70ce..15cbafd04538 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -993,6 +993,49 @@ static int flock_lock_inode(struct inode *inode, struct file_lock *request) return error; } +/** + * posix_chown_locks - change all locks on inode owned by old fl_owner to new + * @file: struct file on which to change the locks + * @old: old fl_owner value to change from + * @new: new fl_owner value to change to + * + * This function changes all of the file locks in an inode owned by an old + * fl_owner to be owned by a new fl_owner. + */ +void posix_chown_locks(struct file *file, fl_owner_t old, fl_owner_t new) +{ + struct inode *inode = file_inode(file); + struct file_lock_context *ctx; + struct file_lock *fl, *tmp; + + /* Don't want to allocate a context in this case */ + ctx = locks_get_lock_context(inode, F_UNLCK); + if (!ctx) + return; + + percpu_down_read_preempt_disable(&file_rwsem); + spin_lock(&ctx->flc_lock); + /* Find the first old lock with the same owner as the new lock */ + list_for_each_entry(fl, &ctx->flc_posix, fl_list) { + if (fl->fl_owner == old) + break; + } + + list_for_each_entry_safe_from(fl, tmp, &ctx->flc_posix, fl_list) { + if (fl->fl_owner != old) + break; + + /* This should only be used for normal userland lockmanager */ + if (fl->fl_lmops) { + WARN_ON_ONCE(1); + continue; + } + fl->fl_owner = new; + } + spin_unlock(&ctx->flc_lock); + percpu_up_read_preempt_enable(&file_rwsem); +} + static int posix_lock_inode(struct inode *inode, struct file_lock *request, struct file_lock *conflock) { diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h index 41615f38bcff..228dfb059d69 100644 --- a/include/linux/fdtable.h +++ b/include/linux/fdtable.h @@ -114,6 +114,7 @@ void do_close_on_exec(struct files_struct *); int iterate_fd(struct files_struct *, unsigned, int (*)(const void *, struct file *, unsigned), const void *); +void change_lock_owners(struct files_struct *, struct files_struct *); extern int __alloc_fd(struct files_struct *files, unsigned start, unsigned end, unsigned flags); diff --git a/include/linux/fs.h b/include/linux/fs.h index 79c413985305..bdc4a8de1af9 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1084,6 +1084,7 @@ extern void locks_remove_posix(struct file *, fl_owner_t); extern void locks_remove_file(struct file *); extern void locks_release_private(struct file_lock *); extern void posix_test_lock(struct file *, struct file_lock *); +extern void posix_chown_locks(struct file *, fl_owner_t old, fl_owner_t new); extern int posix_lock_file(struct file *, struct file_lock *, struct file_lock *); extern int posix_unblock_lock(struct file_lock *); extern int vfs_test_lock(struct file *, struct file_lock *);