From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752796AbcD0FfF (ORCPT ); Wed, 27 Apr 2016 01:35:05 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:36082 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751257AbcD0FfD (ORCPT ); Wed, 27 Apr 2016 01:35:03 -0400 Date: Wed, 27 Apr 2016 06:34:58 +0100 From: Al Viro To: Linus Torvalds Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Miklos Szeredi Subject: Re: [RFC] a corner case of open(2) Message-ID: <20160427053458.GA30149@ZenIV.linux.org.uk> References: <20160426175538.GO25498@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160426175538.GO25498@ZenIV.linux.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Fun bugs caught while trying to massage atomic_open()... Patch below is in vfs.git#for-linus (along with two more fixes); I would like to get an ACK from Miklos on that one - it's his code and this thing had been present in there since the original merge. I might be misreading what it tries to do, but open("/mnt/no-such-file", O_CREAT | O_TRUNC); perror("open"); errno = 0; stat("/mnt/no-such-file", &st); perror("stat"); errno = 0; open("/mnt/no-such-file", O_CREAT | O_TRUNC); perror("open"); should *not* end up with open: Read-only file system stat: No such file or directory open: No such file or directory no matter what. And it's very easy to arrange just that - mount nfs4 read-only on /mnt and run the snippet above. First open() will fail with EROFS (as it should), but as soon as the thing is in dcache we start getting ENOENT. Obviously bogus. commit 1aa57f2aaa108ead7d81481af68085b0a77708f1 Author: Al Viro Date: Wed Apr 27 01:11:55 2016 -0400 atomic_open(): fix the handling of create_error * if we have a hashed negative dentry and either CREAT|EXCL on r/o filesystem, or CREAT|TRUNC on r/o filesystem, or CREAT|EXCL with failing may_o_create(), we should fail with EROFS or the error may_o_create() has returned, but not ENOENT. Which is what the current code ends up returning. * if we have CREAT|TRUNC hitting a regular file on a read-only filesystem, we can't fail with EROFS here. At the very least, not until we'd done follow_managed() - we might have a writable file (or a device, for that matter) bound on top of that one. Moreover, the code downstream will see that O_TRUNC and attempt to grab the write access (*after* following possible mount), so if we really should fail with EROFS, it will happen. No need to do that inside atomic_open(). The real logics is much simpler than what the current code is trying to do - if we decided to go for simple lookup, ended up with a negative dentry *and* had create_error set, fail with create_error. No matter whether we'd got that negative dentry from lookup_real() or had found it in dcache. Cc: stable@vger.kernel.org # v3.6+ Signed-off-by: Al Viro diff --git a/fs/namei.c b/fs/namei.c index 1d9ca2d..b458992 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2942,22 +2942,10 @@ no_open: dentry = lookup_real(dir, dentry, nd->flags); if (IS_ERR(dentry)) return PTR_ERR(dentry); - - if (create_error) { - int open_flag = op->open_flag; - - error = create_error; - if ((open_flag & O_EXCL)) { - if (!dentry->d_inode) - goto out; - } else if (!dentry->d_inode) { - goto out; - } else if ((open_flag & O_TRUNC) && - d_is_reg(dentry)) { - goto out; - } - /* will fail later, go on to get the right error */ - } + } + if (create_error && !dentry->d_inode) { + error = create_error; + goto out; } looked_up: path->dentry = dentry;