From patchwork Tue Oct 31 21:51:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luis Chamberlain X-Patchwork-Id: 10035437 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 4995360327 for ; Tue, 31 Oct 2017 21:52:07 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D99F28B0E for ; Tue, 31 Oct 2017 21:52:07 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 31C0D28B1B; Tue, 31 Oct 2017 21:52:07 +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 5440A28B0E for ; Tue, 31 Oct 2017 21:52:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753342AbdJaVwF (ORCPT ); Tue, 31 Oct 2017 17:52:05 -0400 Received: from mail.kernel.org ([198.145.29.99]:56966 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752473AbdJaVwE (ORCPT ); Tue, 31 Oct 2017 17:52:04 -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 DE10F21890; Tue, 31 Oct 2017 21:52:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DE10F21890 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: sandeen@sandeen.net Cc: dhowells@redhat.com, linux-xfs@vger.kernel.org, "Luis R. Rodriguez" , Tso Ted , Nikolay Borisov , Flex Liu , Jake Norris , Jan Kara Subject: [PATCH] xfs_repair: clear extra file attributes on symlinks Date: Tue, 31 Oct 2017 14:51:56 -0700 Message-Id: <20171031215156.12544-1-mcgrof@kernel.org> X-Mailer: git-send-email 2.13.2 Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@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 as ioctl(2) with FS_IOC_SETFLAGS is used for this purpose, and *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. Given this Linux filesystems should treat extra file attributes set on symbolic links as corruption and fix them. The TL;DR: How I discovered this was finding a corrupted filesystem with symbolic links with the extra file attribute append (STATX_ATTR_APPEND) set. Symbolic links with the attribute append set cannot be removed as they are treated as if a file was set with the immutable flag set. Standard tools however cannot remove *any* attribute flag: # chattr -a symlink chattr: Operation not supported while reading flags on b If you end up with these symbolic links userspace cannot remove them. Likewise one cannot use the same tool to *set* this extra file attribute on a symbolic link using chattr: # rm -f y z # ln -s y z # chattr +a z chattr: Operation not supported while reading flags on z What makes this puzzling was one cannot even list attributes on symlinks using lsattr either: # rm -f a b # ln -s a b # lsattr b lsattr: Operation not supported While reading flags on b The above was due to commit 023d111e92195 ("chattr.1.in: Document the compression attribute flags E, X, and ...") merged on e2fsprogs v1.28 since August 2002. That commit just refers to the fact that attributes were only allowed after that commit for directories and regular files due to Debian bug 152029 [0]. This bug was filed since lsattr lsattr would segfault when used against a special file of an old DRM buggy driver, the ioctl seem to have worked but crashed lsattr with its information. The bug report doesn't list any specific reasoning for not allowing attributes on symlinks though. Crafting your own tool to query the extra file attributes with the new shiny statx(2) works, and if a symbolic link has the extra attribute flag set statx(2) would inform userspace of this. statx(2) is only used for getting file information, not for setting them. This all meant that if you end up with the append extra file attribute set on a symlink you need special tools to try to remove it and currently that's only possible on XFS with xfs_db [1] [2]. Fix XFS filesystems which have these extra file attributes set as the only way they could have been set was through corruption. [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029 [1] xfs_db -x -c 'inode inode-number' -c 'write core.append 1' /dev/device [2] xfs_db -x -c 'inode inode-number' -c 'write core.append 0' /dev/device Cc: Tso Ted Cc: Nikolay Borisov Cc: Flex Liu Cc: Jake Norris Cc: Jan Kara Signed-off-by: Luis R. Rodriguez --- On this v2 I've provided a much better explanation as to why these extra file attributes don't make sense on Linux, and trimmed the flags we venture out to clear to *only* match what statx defines. It may be possible to clear more dino->di_flags and maybe even dino->di_flags2 for symbolic links however that those be determined separately as the other flags' semantics are clarified for setting. repair/dinode.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/repair/dinode.c b/repair/dinode.c index 15ba8cc22b39..6288e42de15e 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), FS_XFLAG_EXTSIZE); } } + if (flags & (XFS_DIFLAG_IMMUTABLE | XFS_DIFLAG_APPEND | + XFS_DIFLAG_NODUMP)) { + /* + * ioctl(fd, *) and so ioctl(fd, FS_IOC_SETFLAGS) + * yields EBADF on symlinks as they have O_PATH set. + * "Extra file attributes", stx_attributes, as per + * statx(2) cannot be set on symlinks on Linux. + */ + if (di_mode && S_ISLNK(di_mode) && + !S_ISREG(di_mode) && !S_ISDIR(di_mode)) { + if (!uncertain) { + do_warn( + _("file or directory attributes set on symlink inode %" PRIu64 "\n"), + lino); + } + flags &= ~(XFS_DIFLAG_IMMUTABLE | + XFS_DIFLAG_APPEND | + XFS_DIFLAG_NODUMP); + } + } + if (!verify_mode && flags != be16_to_cpu(dino->di_flags)) { if (!no_modify) { do_warn(_("fixing bad flags.\n"));