Message-ID: <20180501174512.GA27853@wotan.suse.de>
Date:   Tue, 1 May 2018 17:45:12 +0000
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>, viro@...iv.linux.org.uk,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        sandeen@...deen.net, dhowells@...hat.com, tytso@....edu,
        fliu@...e.com, jack@...e.cz, jeffm@...e.com, nborisov@...e.com,
        jake.norris@...e.com, mtk.manpages@...il.com,
        linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] vfs: skip extra attributes check on removal for symlinks

On Tue, May 01, 2018 at 10:23:19AM -0700, Darrick J. Wong wrote:
> On Thu, Apr 26, 2018 at 04:46:39PM -0700, Luis R. Rodriguez wrote:
> > 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.
> 
> Ah, ok, so the problem here is that you can't rm an "immutable" symlink
> nor can you clear the immutable flag on such a beast, so therefore
> ignore the immutable (and append) flags if we're trying to delete a
> symlink?

Yup.

> I think we ought to teach the xfs inode verifier to check for
> immutable/append symlinks and return error so that we don't end up with
> such things in core in the first place,

Agreed. But note that one way to end up with these things is through
corruption. Once a user finds this the first signs they'll run into
(unless they have the awesome new online scrubber) is they cannot delete
some odd file and not know why. And then they'll see they cannot change
or remove either the immutable or append flag. The immutable attribute
is known to not let you delete the file, but its less known that the
append attribute implies the same. Folks would scratch their heads.

Since one cannot *set* these attributes on symlinks on Linux, other than
ignoring such attributes perhaps we should warn about it? As the only
way you could end up with that is if your filesystem got corrupted.

But without filesystems having a fix for that merged users can't do anything.

> and fix xfs_repair to zap such things.

This is what my patch for xfs_repair does, which is pending review.

But note that special files are handled differently, I explain the logic on the
RFC commit log, which is why I suggested splitting up adding a fix for special
files as a separate patch.

> That said, for the filesystems that aren't going to check their inodes,
> I guess this is a (hackish) way to avoid presenting undeletable gunk in
> the fs to the user...

That's why its RFC. What is the right thing to do here?

My own logic here was since we cannot possibly allow extended attributes on
symlinks is to not use them then as well, in this case for delete.

> (Were it up to me I'd make a common vfs_check_inode() to reject
> struct inode containing garbage that the vfs won't deal with,

There certainly are cases that we could come up with for the VFS where
if such things are found, regardless of the filesystem, we're very sure
its a corruption. This is just one example, as you note.

So it is correct that there are two things here:

1) Do we respect the attribute for symlink on delete
2) Do we warn to users of the fact that the inode is very likely corrupted
   regardless of the filesystem?

This patch only addresses 1) and makes it match the VFS expectation, and
I think this is safe if we're not doing 2). This is why I'm also looking
for feedback from linux-api folks.

This is one of those odd corner cases we run into, and very likely not
well documented anywhere.

> and teach the filesystems to use it;

Or the VFS uses it.

  Luis

Powered by blists - more mailing lists