xfs_repair: clear file / directory attribute on symlinks
RFC 1/1 xfs_repair: clear file / directory attribute on symlinks
symlinks 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
Furthermore one cannot list attributes on symlinks using lsattr either:
# touch a # ln -s a b # lsattr b lsattr: Operation not supported While reading flags on b
This means that if you end up with an append attribute on a symlink you need special tools to try to remove it. This begs the question of whether or not XFS should also disallow attributes on symlinks.
I can trace the lsattr change back down to commit 023d111e92195 ("chattr.1.in: Document the compression attribute flags E, X, and ...") on e2fsprogs 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 used to enable attributes even on special files and lsattr would segfault when used against them since when used against some old DRM buggy driver the ioctl seem to have worked bug crashed lsattr. The bug report doesn't list any specific reasoning for not allowing attributes on symlinks though.
Although in XFS you could end up in a situation where attributes are set on a symlink this is ather hard to do, likewie for clearing them.
Its unclear if XFS purposely supports attributes on symlinks or if this was unintentional.
The ways in which you can end up to setting / unsetting / getting attributes on symlinks on XFS are:
a) corruption b) xfsctl() c) xfs_db [1] [2] - only when umounted
POSIX doesn't seem to actually have a position on this.
These are not extended attributes, however xattr(7) does have a little nugget worth considering regarding this, and for which it was decided to at least not support extended attributes on symlinks or special files:
The file permissions of symbolic links are not used in access checks. These differences would allow users to consume filesystem resources in a way not controllable by disk quotas for group or world writable special files and directories.
For this reason, extended user attributes are allowed only for regular files and directories.
For these reasons, and since corruption is the only current valid case where these seem to have popped up -- apply the same logic on XFS, otherwise we'd be going against the e2fsprogs chattr convention set since August 2002, and which folks likely are used to.
This fixes filesystems which have these attributes set where clearly they were set by 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
---
Otherwise if we *do* wish to support some of these we should document the semantics. Otherwise this seems just odd and users can end up with unexpected situations not being able to easily clear this type of corruption.
The append attribute is particularly annoying given you can't remove these symlinks at all, and the first thing naturally a user could do is try 'lsattr' or 'chattr'. Obviously this won't help and the user will just get frustrated.
repair/dinode.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
repair/dinode.c +35 -0
@@ -2482,6 +2482,41 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), FS_XFLAG_EXTSIZE); } } +if (flags & (XFS_DIFLAG_REALTIME |XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE |XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC |XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP |XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT |XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE |XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG |XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX |XFS_DIFLAG2_COWEXTSIZE )) { +/* chattr cannot clear append on symlink */ +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_REALTIME | +XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE | +XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC | +XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP | +XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT | +XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE | +XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG | +XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX | +XFS_DIFLAG2_COWEXTSIZE ); + } + } +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
Quoted 7 lines · Luis R. Rodriguez
symlinks 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
Quoted 6 lines
Furthermore one cannot list attributes on symlinks using lsattr either:
# touch a # ln -s a b # lsattr b lsattr: Operation not supported While reading flags on b
ok, so tools in e2fsprogs won't clear/set... which is one datapoint...
Quoted 3 lines
This means that if you end up with an append attribute on a symlink you need special tools to try to remove it. This begs the question of whether or not XFS should also disallow attributes on symlinks.
http://begthequestion.info/ ;)
Quoted 10 lines
I can trace the lsattr change back down to commit
023d111e92195("chattr.1.in: Document the compression attribute flags E, X, and ...") on e2fsprogs 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 used to enable attributes even on special files and lsattr would segfault when used against them since when used against some old DRM buggy driver the ioctl seem to have worked bug crashed lsattr. The bug report doesn't list any specific reasoning for not allowing attributes on symlinks though.
Super.
Quoted 21 lines
Although in XFS you could end up in a situation where attributes are set on a symlink this is ather hard to do, likewie for clearing them.
Its unclear if XFS purposely supports attributes on symlinks or if this was unintentional.
The ways in which you can end up to setting / unsetting / getting attributes on symlinks on XFS are:
a) corruption b) xfsctl() c) xfs_db [1] [2] - only when umountedPOSIX doesn't seem to actually have a position on this.
These are not extended attributes, however xattr(7) does have a little nugget worth considering regarding this, and for which it was decided to at least not support extended attributes on symlinks or special files:
Quoted 7 lines
The file permissions of symbolic links are not used in access checks. These differences would allow users to consume filesystem resources in a way not controllable by disk quotas for group or world writable special files and directories.
For this reason, extended user attributes are allowed only for regular files and directories.
Hm, that seems like a different issue/rationale, no?
Quoted 7 lines
For these reasons, and since corruption is the only current valid case where these seem to have popped up -- apply the same logic on XFS, otherwise we'd be going against the e2fsprogs chattr convention set since August 2002, and which folks likely are used to.
This fixes filesystems which have these attributes set where clearly they were set by corruption.
OK but hang on, I think a core question is:
Does the kernel allow us to set attributes on symlinks?
If not, repair should clear them. But if it does, chattr's refusal to clear them is a quirk of that specific tool, not something that xfs_repair needs to worry about ...
If the kernel provides an interface (xfsctl?) to set/clear these attributes, I don't think xfs_repair should be classifying their presence as corruption, right?
If no, really, these should never be set, then the kernel (including the xfsctl route) should reject it - and at that point, I'd be ok with xfs_repair clearing them as "corruption."
-Eric
Quoted 73 lines
[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/deviceCc "Laurent Bonnaud" <[email protected]>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]>---
Otherwise if we *do* wish to support some of these we should document the semantics. Otherwise this seems just odd and users can end up with unexpected situations not being able to easily clear this type of corruption.
The append attribute is particularly annoying given you can't remove these symlinks at all, and the first thing naturally a user could do is try 'lsattr' or 'chattr'. Obviously this won't help and the user will just get frustrated.
repair/dinode.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)repair/dinode.c +35
-0@@ -2482,6 +2482,41 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), FS_XFLAG_EXTSIZE); } } +if (flags & (XFS_DIFLAG_REALTIME |XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE |XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC |XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP |XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT |XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE |XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG |XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX |XFS_DIFLAG2_COWEXTSIZE )) { +/* chattr cannot clear append on symlink */ +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_REALTIME | +XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE | +XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC | +XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP | +XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT | +XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE | +XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG | +XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX | +XFS_DIFLAG2_COWEXTSIZE ); + } + } +if (!verify_mode && flags !=be16_to_cpu (dino->di_flags)) {if (!no_modify) {do_warn (_ ("fixing bad flags.\n" ));
Quoted 34 lines · Eric Sandeen
Quoted 7 lines · Luis R. Rodriguez
symlinks 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 bQuoted 6 lines
Furthermore one cannot list attributes on symlinks using lsattr either:
# touch a # ln -s a b # lsattr b lsattr: Operation not supported While reading flags on bok, so tools in e2fsprogs won't clear/set... which is one datapoint...
Quoted 3 lines
This means that if you end up with an append attribute on a symlink you need special tools to try to remove it. This begs the question of whether or not XFS should also disallow attributes on symlinks.
http://begthequestion.info/ ;)
Quoted 10 lines
I can trace the lsattr change back down to commit
023d111e92195("chattr.1.in: Document the compression attribute flags E, X, and ...") on e2fsprogs 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 used to enable attributes even on special files and lsattr would segfault when used against them since when used against some old DRM buggy driver the ioctl seem to have worked bug crashed lsattr. The bug report doesn't list any specific reasoning for not allowing attributes on symlinks though.
Quoted 1 line
Super.
Ok, so ext4 doesn't support this, nor does e2fprogs... but what about XFS and xfsprogs?
Well, they don't support it either; the point I'm making is that while we support the ext4 SETFLAGS ioctl where we can, what's more important is what the xfs users have been able to do via the FS_IOC_FSSETXATTR ioctl.
Quoted 4 lines
Quoted 4 lines
Although in XFS you could end up in a situation where attributes are set on a symlink this is ather hard to do, likewie for clearing them.
Quoted 8 lines
Quoted 8 lines
Its unclear if XFS purposely supports attributes on symlinks or if this was unintentional.
The ways in which you can end up to setting / unsetting / getting attributes on symlinks on XFS are:
a) corruption b) xfsctl()
Requires an open file descriptor, path argument not used. Returns EBADF if you opened a symlink with O_PATH|O_NOFOLLOW, which AFAIK is the only way to do that.
Quoted 1 line
Quoted 1 line
c)
xfs_db[1] [2] - only when umounted
Fuzzing and corruption aren't all that dissimilar. :)
Quoted 7 lines
Quoted 7 lines
POSIX doesn't seem to actually have a position on this.
These are not extended attributes, however xattr(7) does have a little nugget worth considering regarding this, and for which it was decided to at least not support extended attributes on symlinks or special files:
Quoted 7 lines
Quoted 7 lines
The file permissions of symbolic links are not used in access checks. These differences would allow users to consume filesystem resources in a way not controllable by disk quotas for group or world writable special files and directories.
For this reason, extended user attributes are allowed only for regular files and directories.
Yet lsetxattr() sets extended attributes on a symlink, right?
Quoted 7 lines
Hm, that seems like a different issue/rationale, no?
Quoted 5 lines
For these reasons, and since corruption is the only current valid case where these seem to have popped up -- apply the same logic on XFS, otherwise we'd be going against the e2fsprogs chattr convention set since August 2002, and which folks likely are used to.
Quoted 6 lines
Quoted 2 lines
This fixes filesystems which have these attributes set where clearly they were set by corruption.
OK but hang on, I think a core question is:
Does the kernel allow us to set attributes on symlinks?
I don't think it should, I can't figure out how userspace would set them in the first place, but repair (and now scrub) don't actually check those flags.
Quoted 40 lines
If not, repair should clear them. But if it does, chattr's refusal to clear them is a quirk of that specific tool, not something that
xfs_repairneeds to worry about ...If the kernel provides an interface (xfsctl?) to set/clear these attributes, I don't think
xfs_repairshould be classifying their presence as corruption, right?If no, really, these should never be set, then the kernel (including the xfsctl route) should reject it - and at that point, I'd be ok with
xfs_repairclearing them as "corruption."-Eric
Quoted 26 lines
[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/deviceCc "Laurent Bonnaud" <[email protected]>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]>---
Otherwise if we *do* wish to support some of these we should document the semantics. Otherwise this seems just odd and users can end up with unexpected situations not being able to easily clear this type of corruption.
The append attribute is particularly annoying given you can't remove these symlinks at all, and the first thing naturally a user could do is try 'lsattr' or 'chattr'. Obviously this won't help and the user will just get frustrated.
repair/dinode.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
Quoted 16 lines
Quoted 6 lines
repair/dinode.c +0
-0@@ -2482,6 +2482,41 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), FS_XFLAG_EXTSIZE);repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2492
Quoted 10 lines
repair/dinode.c +8
-0} } +if (flags & (XFS_DIFLAG_REALTIME |XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE |XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC |XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP |XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT |XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE |XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG |XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX |XFS_DIFLAG2_COWEXTSIZE )) {
Do not mix DIFLAG and DIFLAG2.
Also probably easier on the eyes if you don't write this huge OR list twice.
--D
Quoted 35 lines
Quoted 21 lines
repair/dinode.c +20
-0+/* chattr cannot clear append on symlink */ +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_REALTIME | + XFS_DIFLAG_PREALLOC | + XFS_DIFLAG_IMMUTABLE | + XFS_DIFLAG_APPEND | + XFS_DIFLAG_SYNC | + XFS_DIFLAG_NOATIME | + XFS_DIFLAG_NODUMP | + XFS_DIFLAG_RTINHERIT | + XFS_DIFLAG_PROJINHERIT | + XFS_DIFLAG_NOSYMLINKS | + XFS_DIFLAG_EXTSIZE | + XFS_DIFLAG_EXTSZINHERIT |repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2522
Quoted 10 lines
repair/dinode.c +7
-0+XFS_DIFLAG_NODEFRAG | +XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX | + XFS_DIFLAG2_COWEXTSIZE); + } + } +if (!verify_mode && flags !=be16_to_cpu (dino->di_flags)) {if (!no_modify) {do_warn (_ ("fixing bad flags.\n" ));--
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
Quoted 6 lines · Darrick J. Wong
Quoted 1 line · Eric Sandeen
Quoted 1 line
b) xfsctl()Requires an open file descriptor, path argument not used. Returns EBADF if you opened a symlink with O_PATH|O_NOFOLLOW, which AFAIK is the only way to do that.
Hrm, were you suggesting you didn't think its possible to use xfsctl to set some attributes? Because I was able to:
#include <xfs/xfs.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>
char *prog;
static void usage(void)
{
printf("Usage: %s [set|get] <file>\n", prog);
exit(1);
}int main(int argc, char *argv[])
{
int ret = 0;
struct stat sb;
char *cmd_str, *file;
int cmd = 0;
int open_flags = 0;
int fd;
struct fsxattr fsx;
prog = argv[0];
if (argc != 3)
usage();
memset(&fsx, 0, sizeof(fsx));
/* First run is get to get old values */
cmd = FS_IOC_FSGETXATTR;
if ((strncmp("get", argv[1], 3) == 0))
open_flags = O_RDONLY;
else if (strncmp("set", argv[1], 3) == 0) {
cmd = FS_IOC_FSGETXATTR;
open_flags = O_RDWR | O_APPEND;
}
if (!open_flags)
usage();
cmd_str = argv[1];
ret = stat(argv[2], &sb);
if (ret) {
printf("File may not be present or readable\n");
usage();
}
file = argv[2];
fd = open(file, open_flags);
if (!fd) {
printf("Could not open file for operation: %s\n", cmd_str);
usage();
}
ret = xfsctl(file, fd, cmd, &fsx);
if (ret < 0) {
printf("Could not issue xfsctl GET flags\n");
exit(1);
}
if (strncmp("get", argv[1], 3) == 0)
goto out;
/* Now do the setting */
cmd = FS_IOC_FSSETXATTR;
fsx.fsx_xflags |= XFS_XFLAG_APPEND;
ret = xfsctl(file, fd, cmd, &fsx);
if (ret < 0) {
printf("Could not issue xfsctl SET flags\n");
exit(1);
}out:
printf("fsxattr.xflags = 0x%x\n", fsx.fsx_xflags);
return 0;
}However, its unclear if we intentionally supported this.
Quoted 3 lines
Quoted 1 line
Quoted 1 line
c)
xfs_db[1] [2] - only when umountedFuzzing and corruption aren't all that dissimilar. :)
Agreed.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
POSIX doesn't seem to actually have a position on this.
These are not extended attributes, however xattr(7) does have a little nugget worth considering regarding this, and for which it was decided to at least not support extended attributes on symlinks or special files:
The file permissions of symbolic links are not used in access checks. These differences would allow users to consume filesystem resources in a way not controllable by disk quotas for group or world writable special files and directories.
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For this reason, extended user attributes are allowed only for regular files and directories.
Yet
lsetxattr()sets extended attributes on a symlink, right?
True...
Quoted 10 lines
Quoted 10 lines
Hm, that seems like a different issue/rationale, no?
Quoted 7 lines
For these reasons, and since corruption is the only current valid case where these seem to have popped up -- apply the same logic on XFS, otherwise we'd be going against the e2fsprogs chattr convention set since August 2002, and which folks likely are used to.
This fixes filesystems which have these attributes set where clearly they were set by corruption.
Quoted 6 lines
Quoted 3 lines
OK but hang on, I think a core question is:
Does the kernel allow us to set attributes on symlinks?
I don't think it should, I can't figure out how userspace would set them in the first place,
It seems we currently allow for it though :(
Perhaps another consideration to take, but I didn't test, should attributes set with SETFLAGS be preserved across filesystems? If so this may be an issue.
Quoted 1 line
but repair (and now scrub) don't actually check those flags.
Right, given we seem to be rather loose on SETFLAGS, its unclear really what is the right thing to do should be.
Quoted 19 lines
Quoted 8 lines
Quoted 8 lines
repair/dinode.c +0
-0@@ -2482,6 +2482,41 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), FS_XFLAG_EXTSIZE); } }Quoted 8 lines
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2492
Quoted 8 lines
repair/dinode.c +8
-0+if (flags & (XFS_DIFLAG_REALTIME |XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE |XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC |XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP |XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT |XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE |XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG |XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX |XFS_DIFLAG2_COWEXTSIZE )) {Do not mix DIFLAG and DIFLAG2.
Quoted 2 lines
Also probably easier on the eyes if you don't write this huge OR list twice.
Great, I thought it wasn't done before due to some reason, but that is indeed the approach I prefer.
Luis
Quoted 90 lines · Luis R. Rodriguez
Quoted 6 lines · Darrick J. Wong
Quoted 1 line · Eric Sandeen
Quoted 1 line
b) xfsctl()Requires an open file descriptor, path argument not used. Returns EBADF if you opened a symlink with O_PATH|O_NOFOLLOW, which AFAIK is the only way to do that.
Hrm, were you suggesting you didn't think its possible to use xfsctl to set some attributes? Because I was able to:
#include <
xfs/xfs.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>char *prog;
static void usage(void)
{ printf("Usage: %s [set|get] <file>\n", prog); exit(1); }int
main(int argc, char *argv[]){ int ret = 0; struct stat sb; char *cmd_str, *file; int cmd = 0; int open_flags = 0; int fd; struct fsxattr fsx; prog = argv[0]; if (argc != 3) usage(); memset(&fsx, 0, sizeof(fsx)); /* First run is get to get old values */ cmd = FS_IOC_FSGETXATTR; if ((strncmp("get", argv[1], 3) == 0)) open_flags = O_RDONLY; else if (strncmp("set", argv[1], 3) == 0) { cmd = FS_IOC_FSGETXATTR; open_flags = O_RDWR | O_APPEND; } if (!open_flags) usage(); cmd_str = argv[1]; ret = stat(argv[2], &sb); if (ret) { printf("File may not be present or readable\n"); usage(); } file = argv[2]; fd = open(file, open_flags); if (!fd) { printf("Could not open file for operation: %s\n", cmd_str); usage(); } ret = xfsctl(file, fd, cmd, &fsx); if (ret < 0) { printf("Could not issue xfsctl GET flags\n"); exit(1); } if (strncmp("get", argv[1], 3) == 0) goto out; /* Now do the setting */ cmd = FS_IOC_FSSETXATTR; fsx.fsx_xflags |= XFS_XFLAG_APPEND; ret = xfsctl(file, fd, cmd, &fsx); if (ret < 0) { printf("Could not issue xfsctl SET flags\n"); exit(1); }
Quoted 5 lines
out:
printf("fsxattr.xflags = 0x%x\n", fsx.fsx_xflags); return 0; }
I think this program is not really working on the symlink but rather on the file it points to. If you have a dangling symlin then the open() call would just return ENOENT. On the other hand, if the file exists when you open the symlink you will have actually opened the file it points to, no the symlink itself. And this behavior is actually controlled by the vfs traversal code. in path_lookupat:
while (!(err = link_path_walk(s, nd))
&& ((err = lookup_last(nd)) > 0)) {
s = trailing_symlink(nd);
if (IS_ERR(s)) {
err = PTR_ERR(s);
break;
}
}In order to be able to set ioctl-xattr (as per Darick's convetion :D) you need to get a reference to a struct file with it's f_op set to xfs_file_operations, since that's where the unlocked_ioctl handler is. And for symlink that's forbidden by the vfs traversal code.
Quoted 82 lines
However, its unclear if we intentionally supported this.
Quoted 3 lines
Quoted 1 line
Quoted 1 line
c)
xfs_db[1] [2] - only when umountedFuzzing and corruption aren't all that dissimilar. :)
Agreed.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
POSIX doesn't seem to actually have a position on this.
These are not extended attributes, however xattr(7) does have a little nugget worth considering regarding this, and for which it was decided to at least not support extended attributes on symlinks or special files:
The file permissions of symbolic links are not used in access checks. These differences would allow users to consume filesystem resources in a way not controllable by disk quotas for group or world writable special files and directories.
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For this reason, extended user attributes are allowed only for regular files and directories.
Yet
lsetxattr()sets extended attributes on a symlink, right?True...
Quoted 10 lines
Quoted 10 lines
Hm, that seems like a different issue/rationale, no?
Quoted 7 lines
For these reasons, and since corruption is the only current valid case where these seem to have popped up -- apply the same logic on XFS, otherwise we'd be going against the e2fsprogs chattr convention set since August 2002, and which folks likely are used to.
This fixes filesystems which have these attributes set where clearly they were set by corruption.
Quoted 6 lines
Quoted 3 lines
OK but hang on, I think a core question is:
Does the kernel allow us to set attributes on symlinks?
I don't think it should, I can't figure out how userspace would set them in the first place,
It seems we currently allow for it though :(
Perhaps another consideration to take, but I didn't test, should attributes set with SETFLAGS be preserved across filesystems? If so this may be an issue.
Quoted 1 line
but repair (and now scrub) don't actually check those flags.
Right, given we seem to be rather loose on SETFLAGS, its unclear really what is the right thing to do should be.
Quoted 19 lines
Quoted 8 lines
Quoted 8 lines
repair/dinode.c +0
-0@@ -2482,6 +2482,41 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), FS_XFLAG_EXTSIZE); } }Quoted 8 lines
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2492
Quoted 8 lines
repair/dinode.c +8
-0+if (flags & (XFS_DIFLAG_REALTIME |XFS_DIFLAG_PREALLOC | +XFS_DIFLAG_IMMUTABLE |XFS_DIFLAG_APPEND | +XFS_DIFLAG_SYNC |XFS_DIFLAG_NOATIME | +XFS_DIFLAG_NODUMP |XFS_DIFLAG_RTINHERIT | +XFS_DIFLAG_PROJINHERIT |XFS_DIFLAG_NOSYMLINKS | +XFS_DIFLAG_EXTSIZE |XFS_DIFLAG_EXTSZINHERIT | +XFS_DIFLAG_NODEFRAG |XFS_DIFLAG_FILESTREAM | +XFS_DIFLAG2_DAX |XFS_DIFLAG2_COWEXTSIZE )) {Do not mix DIFLAG and DIFLAG2.
Quoted 2 lines
Also probably easier on the eyes if you don't write this huge OR list twice.
Great, I thought it wasn't done before due to some reason, but that is indeed the approach I prefer.
Luis
Quoted 48 lines · Nikolay Borisov
Quoted 45 lines · Luis R. Rodriguez
Quoted 6 lines · Darrick J. Wong
Quoted 1 line · Eric Sandeen
Quoted 1 line
b) xfsctl()Requires an open file descriptor, path argument not used. Returns EBADF if you opened a symlink with O_PATH|O_NOFOLLOW, which AFAIK is the only way to do that.
Hrm, were you suggesting you didn't think its possible to use xfsctl to set some attributes? Because I was able to:
#include <
xfs/xfs.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>char *prog;
static void usage(void)
{ printf("Usage: %s [set|get] <file>\n", prog); exit(1); }int
main(int argc, char *argv[]){ int ret = 0; struct stat sb; char *cmd_str, *file; int cmd = 0; int open_flags = 0; int fd; struct fsxattr fsx; prog = argv[0]; if (argc != 3) usage(); memset(&fsx, 0, sizeof(fsx)); /* First run is get to get old values */ cmd = FS_IOC_FSGETXATTR;
Quoted 7 lines
Quoted 7 lines
if ((strncmp("get", argv[1], 3) == 0)) open_flags = O_RDONLY; else if (strncmp("set", argv[1], 3) == 0) { cmd = FS_IOC_FSGETXATTR; open_flags = O_RDWR | O_APPEND; }
Remove the following chunk:
Quoted 2 lines
Quoted 2 lines
if (!open_flags) usage();
Instead add an:
else usage();
Given O_RDONLY is 0.
Quoted 8 lines
Quoted 8 lines
cmd_str = argv[1]; ret = stat(argv[2], &sb); if (ret) { printf("File may not be present or readable\n"); usage(); }
As per testing stat() won't work on a dangling sylink.
Quoted 8 lines
Quoted 8 lines
file = argv[2]; fd = open(file, open_flags); if (!fd) { printf("Could not open file for operation: %s\n", cmd_str); usage(); }
This open() will though.
Quoted 5 lines
Quoted 5 lines
ret = xfsctl(file, fd, cmd, &fsx); if (ret < 0) { printf("Could not issue xfsctl GET flags\n"); exit(1); }
And this xfsctl() won't work either.
Quoted 16 lines
Quoted 16 lines
if (strncmp("get", argv[1], 3) == 0) goto out; /* Now do the setting */ cmd = FS_IOC_FSSETXATTR; fsx.fsx_xflags |= XFS_XFLAG_APPEND; ret = xfsctl(file, fd, cmd, &fsx); if (ret < 0) { printf("Could not issue xfsctl SET flags\n"); exit(1); }out:
printf("fsxattr.xflags = 0x%x\n", fsx.fsx_xflags);
Quoted 6 lines
Quoted 2 lines
return 0; }I think this program is not really working on the symlink but rather on the file it points to.
I've confirmed this, thanks! Not only does xfsctl() fail on a symlink alone, but also stat(). I'll run some more test but indeed it does not see we currently allow symlinks alone to get user attributes. This however is important information for the commit log so will add it.
Quoted 1 line
If you have a dangling symlin then the
open()call would just return ENOENT.
Actually open() seems to work just fine on a dangling symlink as per my tests.
Quoted 19 lines
On the other hand, if the file exists when you open the symlink you will have actually opened the file it points to, not the symlink itself. And this behavior is actually controlled by the vfs traversal code. in
path_lookupat:while (!(err = link_path_walk(s, nd)) && ((err = lookup_last(nd)) > 0)) { s = trailing_symlink(nd); if (IS_ERR(s)) { err = PTR_ERR(s); break; } }
Quoted 4 lines
In order to be able to set ioctl-xattr (as per Darick's convetion :D) you need to get a reference to a struct file with it's
f_opset toxfs_file_operations, since that's where theunlocked_ioctlhandler is. And for symlink that's forbidden by the vfs traversal code.
Unless someone slipped magic mushrooms on my coffee today open() seems to work. I'll unravel this a bit and add the explanation to why this is not possible on the next patch iteration.
Luis
Quoted 9 lines · Luis R. Rodriguez
Quoted 7 lines
Quoted 7 lines
file = argv[2]; fd = open(file, open_flags); if (!fd) { printf("Could not open file for operation: %s\n", cmd_str); usage(); }This
open()will though.
That's because the special mushrooms I took the day I wrote the above code made me forget to instead check for:
if (fd < 0)
That will always fail.
So open() on a dangling symlink will fail as well.
So it seems we can't set these attributes via userspace, unless you use xfs_db and as noted by Darrick that's not a valid use case, it'd be fuzzing.
Luis