From patchwork Thu Apr 26 23:46:39 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luis Chamberlain X-Patchwork-Id: 10367029 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 8741D60386 for ; Thu, 26 Apr 2018 23:47:01 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 78CC8292A5 for ; Thu, 26 Apr 2018 23:47:01 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 66D36292A9; Thu, 26 Apr 2018 23:47:01 +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=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable 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 06AA1292A5 for ; Thu, 26 Apr 2018 23:47:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751822AbeDZXqq (ORCPT ); Thu, 26 Apr 2018 19:46:46 -0400 Received: from mail.kernel.org ([198.145.29.99]:38154 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751139AbeDZXqp (ORCPT ); Thu, 26 Apr 2018 19:46:45 -0400 Received: from garbanzo.do-not-panic.com (c-73-15-241-2.hsd1.ca.comcast.net [73.15.241.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 556AE217D9; Thu, 26 Apr 2018 23:46:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 556AE217D9 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mcgrof@kernel.org From: "Luis R. Rodriguez" To: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org Cc: sandeen@sandeen.net, darrick.wong@oracle.com, dhowells@redhat.com, tytso@mit.edu, fliu@suse.com, jack@suse.cz, jeffm@suse.com, nborisov@suse.com, jake.norris@suse.com, mtk.manpages@gmail.com, linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org, "Luis R. Rodriguez" Subject: [RFC] vfs: skip extra attributes check on removal for symlinks Date: Thu, 26 Apr 2018 16:46:39 -0700 Message-Id: <20180426234639.12480-1-mcgrof@kernel.org> X-Mailer: git-send-email 2.13.2 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 Linux filesystems cannot set extra file attributes (stx_attributes as per statx(2)) on a symbolic link. To set extra file attributes you issue ioctl(2) with FS_IOC_SETFLAGS, *all* ioctl(2) calls on a symbolic link yield EBADF. This is because ioctl(2) tries to obtain struct fd from the symbolic link file descriptor passed using fdget(), fdget() in turn always returns no file set when a file descriptor is open with O_PATH. As per symlink(2) O_PATH and O_NOFOLLOW must *always* be used when you want to get the file descriptor of a symbolic link, and this holds true for Linux, as such extra file attributes cannot possibly be set on symbolic links on Linux. Filesystems repair utilities should be updated to detect this as corruption and correct this, however, the VFS *does* respect these extra attributes on symlinks for removal. Since we cannot set these attributes we should special-case the immutable/append on delete for symlinks, this would be consistent with what we *do* allow on Linux for all filesystems. The userspace utility chattr(1) cannot set these attributes on symlinks *and* other special files as well: # chattr -a symlink chattr: Operation not supported while reading flags on b The reason for this is different though. Refer to commit 023d111e92195 ("chattr.1.in: Document the compression attribute flags E, X, and ...") merged on e2fsprogs v1.28 since August 2002. This commit prevented issuing the ioctl() for symlink *and* special files in consideration for a buggy DRM driver where issuing lsattr on their special files crashed the system. For details refer to Debian bug 152029 [0]. You can craft your own tool to query the extra file attributes with the new shiny statx(2) tool, statx(2) will list the attributes if they were set for instance on a corrupt filesystem. However statx(2) is only used for *querying* -- not for setting the attributes. If you implement issuing your own ioctl() for FS_IOC_FSGETXATTR or FS_IOC_FSSETXATTR on special files (block, char, fifo) it will fail returning -1 and errno is set to ENOTTY (Inappropriate ioctl for device). The reason for this is different than for symlinks. For special files this fails on vfs_ioctl() when the filesystem f_op callbacks are not set for these special files: long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { int error = -ENOTTY; if (!filp->f_op->unlocked_ioctl) goto out; error = filp->f_op->unlocked_ioctl(filp, cmd, arg); if (error == -ENOIOCTLCMD) error = -ENOTTY; out: return error; } The same applies to PF_LOCAL named sockets. Since this varies by filesystem for special files, only make a special rule to respect the immutable and append attribute on symlinks. [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029 Signed-off-by: Luis R. Rodriguez --- As discussed at LSF/MM -- I'd follow up on this low hanging fruit as the discussion had stalled on linux-xfs on review of the respective xfs_repair changes. This addresses the general API question, and as such I think could help establish order in how we split up patches for those changes. This requires some other eyeballs, and it also requires a putting it through xfstests which I can do in the next few days, hence the RFC. But better put it out for review already. I'd also like feedback from the linux-api folks to see if this matches their own known / documented expectations. fs/namei.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index 186bd2464fd5..0f9069468cfb 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2719,6 +2719,14 @@ int __check_sticky(struct inode *dir, struct inode *inode) } EXPORT_SYMBOL(__check_sticky); +/* Process extra file attributes only when they make sense */ +static bool may_delete_stx_attributes(struct inode *inode) +{ + if (!S_ISLNK(inode->i_mode) && (IS_APPEND(inode) || IS_IMMUTABLE(inode))) + return false; + return true; +} + /* * Check whether we can remove a link victim from directory dir, check * whether the type of victim is right. @@ -2757,8 +2765,8 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir) if (IS_APPEND(dir)) return -EPERM; - if (check_sticky(dir, inode) || IS_APPEND(inode) || - IS_IMMUTABLE(inode) || IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode)) + if (check_sticky(dir, inode) || !may_delete_stx_attributes(inode) || + IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode)) return -EPERM; if (isdir) { if (!d_is_dir(victim))