Re: [PATCH] xfs_repair: clear extra file attributes on symlinks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Oct 31, 2017 at 02:51:56PM -0700, Luis R. Rodriguez wrote:
> 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 <tytso@mit.edu>
> Cc: Nikolay Borisov <nborisov@suse.com>
> Cc: Flex Liu <fliu@suse.com>
> Cc: Jake Norris <jake.norris@suse.com>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> 
> 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)) {

I don't think we can be a link and a file at the same time, right?

Does this DIFLAG clearing applies to bdev/cdev/fifo/socket files too?

--D

> +				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"));
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html