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?
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, and fix xfs_repair to zap such
things.
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...
(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, and teach
the filesystems to use it; but I was shot down when I tried to do that
for negative isize.)
--D
> 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 <mcgrof@xxxxxxxxxx>
> ---
>
> 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))
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
