xfs_repair: clear extra file attributes on symlinks

PATCH Superseded Luis R. Rodriguez 4 participants 12 messages Oct 31, 2017 – Apr 26, 2018

Share & tools

Luis R. Rodriguez

author

2017-10-31 21:52 UTC #0 lore raw

PATCH 1/1 xfs_repair: clear extra file attributes on symlinks

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 <[email protected]>

Cc Nikolay Borisov <[email protected]>

Cc Flex Liu <[email protected]>

Cc Jake Norris <[email protected]>

Cc Jan Kara <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---

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(+)

repair/dinode.c +21 -0

index 15ba8cc22b39..6288e42de15e 100644
@@ -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)) {
+				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"));

Signature · 1 line

2.14.2

Darrick J. Wong

2017-10-31 22:13 UTC #1 lore raw

Quoted 92 lines · Luis R. Rodriguez

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 <[email protected]>
Cc Nikolay Borisov <[email protected]>
Cc Flex Liu <[email protected]>
Cc Jake Norris <[email protected]>
Cc Jan Kara <[email protected]>
Signed-off-by Luis R. Rodriguez <[email protected]>
---
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(+)
repair/dinode.c +0 -0
index 15ba8cc22b39..6288e42de15e 100644
@@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 						FS_XFLAG_EXTSIZE);
 			}
 		}

repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494

Quoted 10 lines

repair/dinode.c +10 -0

+		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

Quoted 21 lines

repair/dinode.c +11 -0

+				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"));

Signature · 6 lines

2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

author

2017-10-31 22:19 UTC #2 lore raw

Quoted 97 lines · Darrick J. Wong

Quoted 96 lines · Luis R. Rodriguez
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 <[email protected]>
Cc Nikolay Borisov <[email protected]>
Cc Flex Liu <[email protected]>
Cc Jake Norris <[email protected]>
Cc Jan Kara <[email protected]>
Signed-off-by Luis R. Rodriguez <[email protected]>
---
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(+)
repair/dinode.c +4 -0
index 15ba8cc22b39..6288e42de15e 100644
@@ -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)

Quoted 8 lines

repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494

Quoted 6 lines

repair/dinode.c +6 -0

+                      * 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?

True, the check should be much simpler with just S_ISLNK().

Quoted 1 line

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

Not at the moment given the semantics I hunted down and tested for were for O_PATH only. The validation I hunted down applies to any file descriptors which we open via O_PATH only.

Recall that the Debian bug that Ted had fixed in userspace was a work
around for userspace querying the file extra attributes via ioctl onto a buggy DRM driver, so a special file. The *setting* via ioctl() can certainly be ruled out for O_PATH files, but for other types of files other types of evaluations would be needed.

Luis

Luis R. Rodriguez

author

2017-10-31 22:41 UTC #3 lore raw

PATCH v2 1/1 xfs_repair: clear extra file attributes on symlinks

Given this Linux filesystems should treat extra file attributes set on symbolic links as corruption and fix them.

The TL;DR:

	# 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

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.

Cc Tso Ted <[email protected]>

Cc Nikolay Borisov <[email protected]>

Cc Flex Liu <[email protected]>

Cc Jake Norris <[email protected]>

Cc Jan Kara <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---

On this v2:

As per Darrick, just only use S_ISLNK(), that should suffice.

 repair/dinode.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

repair/dinode.c +20 -0

index 15ba8cc22b39..33050cbe9ae8 100644
@@ -2482,6 +2482,26 @@ _("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)) {
+				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"));

Signature · 1 line

2.14.2

Darrick J. Wong

2017-10-31 22:43 UTC #4 lore raw

In reply to Luis R. Rodriguez

Quoted 108 lines · Luis R. Rodriguez

Quoted 97 lines · Darrick J. Wong
Quoted 96 lines · Luis R. Rodriguez
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 <[email protected]>
Cc Nikolay Borisov <[email protected]>
Cc Flex Liu <[email protected]>
Cc Jake Norris <[email protected]>
Cc Jan Kara <[email protected]>
Signed-off-by Luis R. Rodriguez <[email protected]>
---
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(+)
repair/dinode.c +4 -0
index 15ba8cc22b39..6288e42de15e 100644
@@ -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)
Quoted 8 lines
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494
Quoted 6 lines
repair/dinode.c +6 -0
+                      * 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?

Quoted 7 lines

True, the check should be much simpler with just S_ISLNK().
Quoted 1 line
Does this DIFLAG clearing applies to bdev/cdev/fifo/socket files too?
Not at the moment given the semantics I hunted down and tested for were for O_PATH only. The validation I hunted down applies to any file descriptors which we open via O_PATH only.

iirc when you open one of those special files you end up with a fd that points to an inode on a special bdevfs/pipefs/etc., not an inode linked to the underlying filesystem containing the special file. Therefore, you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.

# mknod block b 8 0 ; mknod char c 1 3 ; mknod fifo p
# lsattr block char fifo
lsattr: Operation not supported While reading flags on block
lsattr: Operation not supported While reading flags on char
lsattr: Operation not supported While reading flags on fifo

--D

Quoted 11 lines

Recall that the Debian bug that Ted had fixed in userspace was a work
around for userspace querying the file extra attributes via ioctl onto a buggy DRM driver, so a special file. The *setting* via ioctl() can certainly be ruled out for O_PATH files, but for other types of files other types of evaluations would be needed.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez

author

2017-10-31 23:10 UTC #5 lore raw

Quoted 25 lines · Darrick J. Wong

Quoted 24 lines · Luis R. Rodriguez

Quoted 21 lines · Darrick J. Wong

Quoted 8 lines · Luis R. Rodriguez

repair/dinode.c +0 -0

index 15ba8cc22b39..6288e42de15e 100644
@@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
                                              FS_XFLAG_EXTSIZE);
                      }
              }

repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494

Quoted 10 lines

repair/dinode.c +10 -0

+             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)) {

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

Quoted 7 lines

Quoted 3 lines
Not at the moment given the semantics I hunted down and tested for were for O_PATH only. The validation I hunted down applies to any file descriptors which we open via O_PATH only.
iirc when you open one of those special files you end up with a fd that points to an inode on a special bdevfs/pipefs/etc., not an inode linked to the underlying filesystem containing the special file.

That seems to fit the O_PATH intent, however its unclear if O_PATH was needed, as per my testing on /dev/loop0 I don't need O_PATH set for it.

Quoted 1 line

Therefore you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.

That would be great if we can verify.

Quoted 5 lines

# mknod block b 8 0 ; mknod char c 1 3 ; mknod fifo p
# lsattr block char fifo
lsattr: Operation not supported While reading flags on block
lsattr: Operation not supported While reading flags on char
lsattr: Operation not supported While reading flags on fifo

I'm afraid e2fsprogs has a special check for these, ie, userspace is barred from actually toying with special files purposely because of the Debian bug I named.

strace should reveal the respective ioctl() was not actually issued.

I just tested a stupid program against /dev/loop0 and it fails, but not because of O_PATH and EBADF being returned, somewhere in the path EINVAL is returned, question is where and why.

openat(AT_FDCWD, "/dev/loop0", O_RDONLY) = 3
ioctl(3, FS_IOC_FSGETXATTR, 0x7ffde0d82d40) = -1 EINVAL (Invalid argument)

I'm happy to fold another patch in for these but it seems to me the logic is a bit different and the special checks should be confirmed.

Luis

Eric Sandeen

2017-10-31 23:12 UTC #6 lore raw

Quoted 47 lines · Luis R. Rodriguez

Quoted 25 lines · Darrick J. Wong
Quoted 24 lines · Luis R. Rodriguez
Quoted 21 lines · Darrick J. Wong
Quoted 8 lines · Luis R. Rodriguez
repair/dinode.c +0 -0
index 15ba8cc22b39..6288e42de15e 100644
@@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
                                              FS_XFLAG_EXTSIZE);
                      }
              }
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494
Quoted 10 lines
repair/dinode.c +10 -0
+             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)) {
Does this DIFLAG clearing applies to bdev/cdev/fifo/socket files too?
Quoted 7 lines
Quoted 3 lines
Not at the moment given the semantics I hunted down and tested for were for O_PATH only. The validation I hunted down applies to any file descriptors which we open via O_PATH only.
iirc when you open one of those special files you end up with a fd that points to an inode on a special bdevfs/pipefs/etc., not an inode linked to the underlying filesystem containing the special file.
That seems to fit the O_PATH intent, however its unclear if O_PATH was needed, as per my testing on /dev/loop0 I don't need O_PATH set for it.
Quoted 1 line
Therefore you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.
That would be great if we can verify.
Quoted 5 lines
# mknod block b 8 0 ; mknod char c 1 3 ; mknod fifo p
# lsattr block char fifo
lsattr: Operation not supported While reading flags on block
lsattr: Operation not supported While reading flags on char
lsattr: Operation not supported While reading flags on fifo

Quoted 5 lines

I'm afraid e2fsprogs has a special check for these, ie, userspace is barred from actually toying with special files purposely because of the Debian bug I named.
strace should reveal the respective ioctl() was not actually issued.

That's very easy to short-circuit in e2fsprogs if you'd like to test what the kernel does today.

-Eric

Luis R. Rodriguez

author

2017-11-01 23:50 UTC #7 lore raw

Quoted 50 lines · Eric Sandeen

Quoted 47 lines · Luis R. Rodriguez
Quoted 25 lines · Darrick J. Wong
Quoted 24 lines · Luis R. Rodriguez
Quoted 21 lines · Darrick J. Wong
Quoted 8 lines · Luis R. Rodriguez
repair/dinode.c +0 -0
index 15ba8cc22b39..6288e42de15e 100644
@@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
                                              FS_XFLAG_EXTSIZE);
                      }
              }
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494
Quoted 10 lines
repair/dinode.c +10 -0
+             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)) {
Does this DIFLAG clearing applies to bdev/cdev/fifo/socket files too?
Quoted 7 lines
Quoted 3 lines
Not at the moment given the semantics I hunted down and tested for were for O_PATH only. The validation I hunted down applies to any file descriptors which we open via O_PATH only.
iirc when you open one of those special files you end up with a fd that points to an inode on a special bdevfs/pipefs/etc., not an inode linked to the underlying filesystem containing the special file.
That seems to fit the O_PATH intent, however its unclear if O_PATH was needed, as per my testing on /dev/loop0 I don't need O_PATH set for it.
Quoted 1 line
Therefore you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.
That would be great if we can verify.
Quoted 5 lines
# mknod block b 8 0 ; mknod char c 1 3 ; mknod fifo p
# lsattr block char fifo
lsattr: Operation not supported While reading flags on block
lsattr: Operation not supported While reading flags on char
lsattr: Operation not supported While reading flags on fifo

Quoted 8 lines

Quoted 5 lines
I'm afraid e2fsprogs has a special check for these, ie, userspace is barred from actually toying with special files purposely because of the Debian bug I named.
strace should reveal the respective ioctl() was not actually issued.
That's very easy to short-circuit in e2fsprogs if you'd like to test what the kernel does today.

I meant that *I know* that the ioctl() is not issued, as it is a guard implemented on e2fsprogs to avoid potentially interfacing with buggy kernel drivers.

One however can write a program which does the raw ioctl() call to really test.

For all the above: block char fifo, ioctl() with FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR fails with: -1 an errno is set to ENOTTY (Inappropriate ioctl for device). Reason is that vfs_ioctl() is used, and on XFS we'll hit the goto out here as no f_op callbacks are set for these special devices:

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;                                                           
}

My point also was that the above logic is different than for symlink. I'd much prefer to address these as a secondary patch given the logic is different.

PF_LOCAL named sockets are also represented on the filesystem, and same situation with them, so I can bunch up a check for them as well. Are you OK with these going in as separate patches or do you want me to mesh this all together along with the new explanation for these?

Luis

Luis R. Rodriguez

author

2018-04-26 23:50 UTC #8 lore raw

Quoted 84 lines · Luis R. Rodriguez

Quoted 50 lines · Eric Sandeen
Quoted 47 lines · Luis R. Rodriguez
Quoted 25 lines · Darrick J. Wong
Quoted 24 lines · Luis R. Rodriguez
Quoted 21 lines · Darrick J. Wong
Quoted 8 lines · Luis R. Rodriguez
repair/dinode.c +0 -0
index 15ba8cc22b39..6288e42de15e 100644
@@ -2482,6 +2482,27 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
                                              FS_XFLAG_EXTSIZE);
                      }
              }
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2494
Quoted 10 lines
repair/dinode.c +10 -0
+             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)) {
Does this DIFLAG clearing applies to bdev/cdev/fifo/socket files too?
Quoted 7 lines
Quoted 3 lines
Not at the moment given the semantics I hunted down and tested for were for O_PATH only. The validation I hunted down applies to any file descriptors which we open via O_PATH only.
iirc when you open one of those special files you end up with a fd that points to an inode on a special bdevfs/pipefs/etc., not an inode linked to the underlying filesystem containing the special file.
That seems to fit the O_PATH intent, however its unclear if O_PATH was needed, as per my testing on /dev/loop0 I don't need O_PATH set for it.
Quoted 1 line
Therefore you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.
That would be great if we can verify.
Quoted 5 lines
# mknod block b 8 0 ; mknod char c 1 3 ; mknod fifo p
# lsattr block char fifo
lsattr: Operation not supported While reading flags on block
lsattr: Operation not supported While reading flags on char
lsattr: Operation not supported While reading flags on fifo
Quoted 8 lines
Quoted 5 lines
I'm afraid e2fsprogs has a special check for these, ie, userspace is barred from actually toying with special files purposely because of the Debian bug I named.
strace should reveal the respective ioctl() was not actually issued.
That's very easy to short-circuit in e2fsprogs if you'd like to test what the kernel does today.
I meant that *I know* that the ioctl() is not issued, as it is a guard implemented on e2fsprogs to avoid potentially interfacing with buggy kernel drivers.
One however can write a program which does the raw ioctl() call to really test.
For all the above: block char fifo, ioctl() with FS_IOC_FSGETXATTR and FS_IOC_FSSETXATTR fails with: -1 an errno is set to ENOTTY (Inappropriate ioctl for device). Reason is that vfs_ioctl() is used, and on XFS we'll hit the goto out here as no f_op callbacks are set for these special devices:
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;                                                           
}

Quoted 7 lines

My point also was that the above logic is different than for symlink. I'd much prefer to address these as a secondary patch given the logic is different.
PF_LOCAL named sockets are also represented on the filesystem, and same situation with them, so I can bunch up a check for them as well. Are you OK with these going in as separate patches or do you want me to mesh this all together along with the new explanation for these?

*re-poke*

I hope the VFS RFC patch I just posted clarifies the situation a bit more.

Luis

Luis R. Rodriguez

author

2017-11-02 00:39 UTC #9 lore raw

In reply to Darrick J. Wong

Quoted 1 line · Darrick J. Wong

you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.

Do we know for certain *all* DIFLAG/DIFLAG2 flags can *only* be set via an ioctl()?

Luis

Darrick J. Wong

2017-11-02 05:23 UTC #10 lore raw

Quoted 5 lines · Luis R. Rodriguez

Quoted 1 line · Darrick J. Wong
you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.
Do we know for certain *all* DIFLAG/DIFLAG2 flags can *only* be set via an ioctl()?

I'm fairly sure that inode flags get set via ioctl, via inheritance at creation time with only two exceptions*, but could you please take a look at all the places we set inode di_flags/di_flags2 and report back?

--D

* The DAX flag can get set (I think?) via the mount option; and the REFLINK flag can be set on files via copy_file_range().

Quoted 5 lines

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dave Chinner

2017-11-02 21:22 UTC #11 lore raw

Quoted 10 lines · Darrick J. Wong

Quoted 5 lines · Luis R. Rodriguez
Quoted 1 line · Darrick J. Wong
you shouldn't be able to set any DIFLAG/DIFLAG2 flags on special files.
Do we know for certain *all* DIFLAG/DIFLAG2 flags can *only* be set via an ioctl()?
I'm fairly sure that inode flags get set via ioctl, via inheritance at creation time with only two exceptions*, but could you please take a look at all the places we set inode di_flags/di_flags2 and report back?

*users* can only set/clear inode flags through ioctls. The kernel can set/clear inode flags at any time, and they may be flags that users cannot change (e.g. the reflink flag, or the prealloc flag which is set during fallocate() calls to tell the kernel not to remove preallocations beyond EOF).

Write up a set of masks that state what flags are valid on what type of inode. We can review that for correctness, then we can work out how to handle bad flags in repair...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]