lkml.org 
[lkml]   [2018]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] vfs: skip extra attributes check on removal for symlinks
    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@kernel.org>
    > ---
    >
    > 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@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html

    \
     
     \ /
      Last update: 2018-05-01 19:24    [W:3.922 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site