Filesystems which detect symlinks with append/immutable should inform users their filesystem is corrupted and their respective filesystem checker tool should fix this. In lieu of this though users may be stuck with pesky files or directories which they cannot remove. We cannot expect all filesystems to be updated to address this, so since the VFS does not let filesystems set these attributes -- let the VFS enable users to remove symlink with these attributes, but also provide a fallback warning, in case the users's own filesystem does not catch it. Sending again as RFC as this just goes compile tested so far, and it is still unclear if this is the direction we want to go with this. v2: As per Darrick, even though the VFS should probably allow not so popular filesystems to delete corrupt symlinks with append/immutable -- popular filesystems should check for this themselves and inform the users of corruption. These filesystems should have their respective filesystem checker tools updated to correct this as well. v1: Sent out a single patch just to ignore the append/immutable attributes set on symlinks. Luis R. Rodriguez (4): vfs: skip extra attributes check on removal for symlinks xfs: add verifier check for symlink with append/immutable flags ext4: add verifier check for symlink with append/immutable flags btrfs: verify symlinks with append/immutable flags fs/btrfs/inode.c | 9 +++++++++ fs/ext4/inode.c | 7 +++++++ fs/namei.c | 24 ++++++++++++++++++++++-- fs/xfs/libxfs/xfs_symlink_remote.c | 5 +++++ 4 files changed, 43 insertions(+), 2 deletions(-) -- 2.17.0
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. Since this is a clear sign to the VFS the filesystem must be corrupted filesystems can implement a verifier to catch this earlier. A generic warning issued for filesystems which don't implement these verifiers, and the VFS also lets users delete these pesky symlinks as otherwise users cannot get rid of them. 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> --- fs/namei.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index e861b409c241..23ebc14805dc 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2760,6 +2760,26 @@ 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) +{ + /* + * The VFS does not allow setting append/immutable on symlinks. + * + * Filesystems can implement their own verifier which would avoid this + * generic splat, this generic splat is desirable if the respective + * filesystem repair utility won't implement a fix for this, otherwise + * users end up with a nagging dangling file which is impossible to + * fix in userspace. + */ + if (S_ISLNK(inode->i_mode)) { + WARN_ONCE((IS_APPEND(inode) || IS_IMMUTABLE(inode)), + "Immutable or append flag set on symlink. VFS does not allow this, must be a filesystem corruption. Allowing deletion though"); + } else if (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. @@ -2798,8 +2818,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
The Linux VFS does not allow a way to set append/immuttable attributes to symlinks, this is just not possible. If this is detected we can correct this with xfs_repair, so inform the user. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- fs/xfs/libxfs/xfs_symlink_remote.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c index 5ef5f354587e..42dd81ede3d6 100644 --- a/fs/xfs/libxfs/xfs_symlink_remote.c +++ b/fs/xfs/libxfs/xfs_symlink_remote.c @@ -242,5 +242,10 @@ xfs_symlink_shortform_verify( /* We /did/ null-terminate the buffer, right? */ if (*endp != 0) return __this_address; + + /* Immutable and append flags are not allowed on symlinks */ + if (ip->i_d.di_flags & (XFS_DIFLAG_APPEND | XFS_DIFLAG_IMMUTABLE)) + return __this_address; + return NULL; } -- 2.17.0
The Linux VFS does not allow a way to set append/immuttable attributes to symlinks, this is just not possible. If this is detected inform the user as the filesystem must be corrupted. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- fs/ext4/inode.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 37a2f7a2b66a..6acf0dd6b6e6 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -4947,6 +4947,13 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) inode->i_op = &ext4_dir_inode_operations; inode->i_fop = &ext4_dir_operations; } else if (S_ISLNK(inode->i_mode)) { + /* VFS does not allow setting these so must be corruption */ + if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) { + EXT4_ERROR_INODE(inode, + "immutable or append flags not allowed on symlinks"); + ret = -EFSCORRUPTED; + goto bad_inode; + } if (ext4_encrypted_inode(inode)) { inode->i_op = &ext4_encrypted_symlink_inode_operations; ext4_set_aops(inode); -- 2.17.0
The Linux VFS does not allow a way to set append/immuttable attributes to symlinks, this is just not possible. If this is detected inform the user as the filesystem must be corrupted. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- fs/btrfs/inode.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index c4bdb597b323..d9c786be408c 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3933,6 +3933,15 @@ static int btrfs_read_locked_inode(struct inode *inode) inode->i_op = &btrfs_dir_inode_operations; break; case S_IFLNK: + /* VFS does not allow setting these so must be corruption */ + if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) { + ret = -EUCLEAN; + btrfs_crit(fs_info, + "corrupt symlink with append/immutable ino=%llu root=%llu\n", + btrfs_ino(BTRFS_I(inode)), + root->root_key.objectid); + goto make_bad; + } inode->i_op = &btrfs_symlink_inode_operations; inode_nohighmem(inode); inode->i_mapping->a_ops = &btrfs_symlink_aops; -- 2.17.0
On Thu, May 10, 2018 at 04:13:57PM -0700, Luis R. Rodriguez wrote: > The Linux VFS does not allow a way to set append/immuttable attributes > to symlinks, this is just not possible. If this is detected we can > correct this with xfs_repair, so inform the user. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > fs/xfs/libxfs/xfs_symlink_remote.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c > index 5ef5f354587e..42dd81ede3d6 100644 > --- a/fs/xfs/libxfs/xfs_symlink_remote.c > +++ b/fs/xfs/libxfs/xfs_symlink_remote.c > @@ -242,5 +242,10 @@ xfs_symlink_shortform_verify( > /* We /did/ null-terminate the buffer, right? */ > if (*endp != 0) > return __this_address; > + > + /* Immutable and append flags are not allowed on symlinks */ > + if (ip->i_d.di_flags & (XFS_DIFLAG_APPEND | XFS_DIFLAG_IMMUTABLE)) > + return __this_address; This belongs in xfs_dinode_verify so that it checks all symlinks, not just the one shortform ones. --D > + > return NULL; > } > -- > 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
On Thu, May 10, 2018 at 04:13:59PM -0700, Luis R. Rodriguez wrote: > The Linux VFS does not allow a way to set append/immuttable ^^^^^^^^^^ Typo, in all 3 patches. > attributes to symlinks, this is just not possible. If this is > detected inform the user as the filesystem must be corrupted. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > fs/btrfs/inode.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index c4bdb597b323..d9c786be408c 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3933,6 +3933,15 @@ static int btrfs_read_locked_inode(struct inode *inode) > inode->i_op = &btrfs_dir_inode_operations; > break; > case S_IFLNK: > + /* VFS does not allow setting these so must be corruption */ > + if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) { > + ret = -EUCLEAN; > + btrfs_crit(fs_info, > + "corrupt symlink with append/immutable ino=%llu root=%llu\n", no "\n" and please un-indent the string so it fits 80 columns. > + btrfs_ino(BTRFS_I(inode)), > + root->root_key.objectid); > + goto make_bad; I found some error handling issues, before the switch, there's btrfs_free_path and there's one more at the make_bad label. To fix that, please set path = NULL after the first btrfs_free_path, it can handle a NULL when it's called again. Next thing I'm not sure about are the ACLs that get initialized in some cases. There's cache_no_acl() that only resets the inode::i_acl and inode::i_default_acl, so I think this should be called too. Thanks.
On Thu 10-05-18 16:13:58, Luis R. Rodriguez wrote: > The Linux VFS does not allow a way to set append/immuttable > attributes to symlinks, this is just not possible. If this is > detected inform the user as the filesystem must be corrupted. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> Looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/inode.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 37a2f7a2b66a..6acf0dd6b6e6 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4947,6 +4947,13 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) > inode->i_op = &ext4_dir_inode_operations; > inode->i_fop = &ext4_dir_operations; > } else if (S_ISLNK(inode->i_mode)) { > + /* VFS does not allow setting these so must be corruption */ > + if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) { > + EXT4_ERROR_INODE(inode, > + "immutable or append flags not allowed on symlinks"); > + ret = -EFSCORRUPTED; > + goto bad_inode; > + } > if (ext4_encrypted_inode(inode)) { > inode->i_op = &ext4_encrypted_symlink_inode_operations; > ext4_set_aops(inode); > -- > 2.17.0 > -- Jan Kara <jack@suse.com> SUSE Labs, CR
On Fri, May 11, 2018 at 11:12:18PM +0200, Jan Kara wrote:
> On Thu 10-05-18 16:13:58, Luis R. Rodriguez wrote:
> > The Linux VFS does not allow a way to set append/immuttable
> > attributes to symlinks, this is just not possible. If this is
> > detected inform the user as the filesystem must be corrupted.
> >
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>
> Looks good to me. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
Applied into the ext4 tree after verifying that e2fsck already handles
this case:
% e2fsck -fy /tmp/foo.img
e2fsck 1.44.1 (24-Mar-2018)
Pass 1: Checking inodes, blocks, and sizes
Special (device/socket/fifo/symlink) file (inode 13) has immutable
or append-only flag set. Clear? yes
(The btrfs and xfs maintainers might want to make a similar check
before accepting their respective patches.)
- Ted