xfs: fix reporting supported extra file attributes for statx()
RFC 1/1 xfs: fix reporting supported extra file attributes for statx()
statx(2) notes that any attribute that is not indicated as supported by stx_attributes_mask has no usable value. Commit 5f955f26f3d42d ("xfs: report crtime and attribute flags to statx") added support for informing userspace of extra file attributes but forgot to list these flags as supported making reporting them rather useless for the pedantic userspace author.
$ git describe --contains 5f955f26f3d42d04aba65590a32eb70eedb7f37d v4.11-rc6~5^2^2~2
---
Its unclear why David left these out or if it was just a mistake, I checked the original patch thread [0] but it was not clear in the end. David?
Also, I posted a patch to add support to clearing these flags through xfs_repair on symlinks [1] due to the pain this can cause as you have no option but to use xfs_db to fix these otherwise. Since we *didn't* list these extra file attributes as supported, it begs the question if instead we should only set them *and* this mask if !S_ISLNK(inode->i_mode)).
If so, that also begs the question if we should add further sanity check on the xfs setattr to ensure these are never set on symbolic links, despite the fact that FS_IOC_FSSETXATTR ioctl won't be able to set them...
A long shot question in terms of semantics is if all the above is rather generic for Linux, is if the VFS should even be checking for immutable or append flags on unlink for symbolic links.
[0] https://patchwork.kernel.org/patch/9607879/
[1] https://lkml.kernel.org/r/[email protected]
fs/xfs/xfs_iops.c | 4 ++++ 1 file changed, 4 insertions(+)
fs/xfs/xfs_iops.c +4 -0
@@ -531,6 +531,10 @@ xfs_vn_getattr(if (ip->i_d.di_flags &XFS_DIFLAG_NODUMP ) stat->attributes |=STATX_ATTR_NODUMP ; + stat->attributes_mask |= (STATX_ATTR_IMMUTABLE | +STATX_ATTR_APPEND | +STATX_ATTR_NODUMP ); +switch (inode->i_mode &S_IFMT ) {case S_IFBLK :case S_IFCHR :
Signature · 1 line
2.14.2
Quoted 5 lines · Luis R. Rodriguez
statx(2) notes that any attribute that is not indicated as supported by
stx_attributes_maskhas no usable value. Commit5f955f26f3d42d("xfs: report crtime and attribute flags to statx") added support for informing userspace of extra file attributes but forgot to list these flags as supported making reporting them rather useless for the pedantic userspace author.
Maybe the vfs should WARN_ON(stat->attributes & ~stat->attributes_mask); to prevent us from screwing this up further?
Quoted 16 lines
$ git describe --contains 5f955f26f3d42d04aba65590a32eb70eedb7f37d v4.11-rc6~5^2^2~2Fixes 5f955f26f3d42d ("xfs: report crtime and attribute flags to statx")Signed-off-by Luis R. Rodriguez <[email protected]>---
Its unclear why David left these out or if it was just a mistake, I checked the original patch thread [0] but it was not clear in the end. David?
Also, I posted a patch to add support to clearing these flags through
xfs_repairon symlinks [1] due to the pain this can cause as you have no option but to usexfs_dbto fix these otherwise. Since we *didn't* list these extra file attributes as supported, it begs the question if instead we should only set them *and* this mask if !S_ISLNK(inode->i_mode)).
Quoted 3 lines
If so, that also begs the question if we should add further sanity check on the xfs setattr to ensure these are never set on symbolic links, despite the fact that
FS_IOC_FSSETXATTRioctl won't be able to set them...
Sure.
--D
Quoted 32 lines
A long shot question in terms of semantics is if all the above is rather generic for Linux, is if the VFS should even be checking for immutable or append flags on unlink for symbolic links.
[0] https://patchwork.kernel.org/patch/9607879/
[1] https://lkml.kernel.org/r/[email protected]fs/xfs/xfs_iops.c | 4 ++++ 1 file changed, 4 insertions(+)fs/xfs/xfs_iops.c +4
-0@@ -531,6 +531,10 @@ xfs_vn_getattr(if (ip->i_d.di_flags &XFS_DIFLAG_NODUMP ) stat->attributes |=STATX_ATTR_NODUMP ; + stat->attributes_mask |= (STATX_ATTR_IMMUTABLE | +STATX_ATTR_APPEND | +STATX_ATTR_NODUMP ); +switch (inode->i_mode &S_IFMT ) {case S_IFBLK :case S_IFCHR :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
Quoted 9 lines · Darrick J. Wong
Quoted 5 lines · Luis R. Rodriguez
statx(2) notes that any attribute that is not indicated as supported by
stx_attributes_maskhas no usable value. Commit5f955f26f3d42d("xfs: report crtime and attribute flags to statx") added support for informing userspace of extra file attributes but forgot to list these flags as supported making reporting them rather useless for the pedantic userspace author.Maybe the vfs should
WARN_ON(stat->attributes & ~stat->attributes_mask); to prevent us from screwing this up further?
Its unclear, what was the mask devised for then, was it envisioned that there will be filesystems that will set some values but yet purposely list them as unsupported?
Quoted 16 lines
Quoted 16 lines
$ git describe --contains 5f955f26f3d42d04aba65590a32eb70eedb7f37d v4.11-rc6~5^2^2~2Fixes 5f955f26f3d42d ("xfs: report crtime and attribute flags to statx")Signed-off-by Luis R. Rodriguez <[email protected]>---
Its unclear why David left these out or if it was just a mistake, I checked the original patch thread [0] but it was not clear in the end. David?
Also, I posted a patch to add support to clearing these flags through
xfs_repairon symlinks [1] due to the pain this can cause as you have no option but to usexfs_dbto fix these otherwise. Since we *didn't* list these extra file attributes as supported, it begs the question if instead we should only set them *and* this mask if !S_ISLNK(inode->i_mode)).
Quoted 5 lines
Quoted 3 lines
If so, that also begs the question if we should add further sanity check on the xfs setattr to ensure these are never set on symbolic links, despite the fact that
FS_IOC_FSSETXATTRioctl won't be able to set them...Sure.
Alright, I'll let things wind down and if I don't hear otherwise back will make the change.
Luis
Quoted 9 lines · Luis R. Rodriguez
Quoted 7 lines · Darrick J. Wong
Quoted 5 lines · Luis R. Rodriguez
statx(2) notes that any attribute that is not indicated as supported by
stx_attributes_maskhas no usable value. Commit5f955f26f3d42d("xfs: report crtime and attribute flags to statx") added support for informing userspace of extra file attributes but forgot to list these flags as supported making reporting them rather useless for the pedantic userspace author.
Quoted 6 lines
Quoted 2 lines
Maybe the vfs should
WARN_ON(stat->attributes & ~stat->attributes_mask); to prevent us from screwing this up further?Its unclear, what was the mask devised for then, was it envisioned that there will be filesystems that will set some values but yet purposely list them as unsupported?
It's for flags like ENCRYPTED so userspace can distinguish between the bit being unset meaning that the file is plaintext vs. the filesystem not knowing what encrypted files are at all.
(Well ok it'd probably be more useful for a NOCOW bit so that you can distinguish a COW file vs. a filesystem that doesn't COW at all, but evidently that isn't wired up.)
--D
Quoted 30 lines
Quoted 16 lines
Quoted 16 lines
$ git describe --contains 5f955f26f3d42d04aba65590a32eb70eedb7f37d v4.11-rc6~5^2^2~2Fixes 5f955f26f3d42d ("xfs: report crtime and attribute flags to statx")Signed-off-by Luis R. Rodriguez <[email protected]>---
Its unclear why David left these out or if it was just a mistake, I checked the original patch thread [0] but it was not clear in the end. David?
Also, I posted a patch to add support to clearing these flags through
xfs_repairon symlinks [1] due to the pain this can cause as you have no option but to usexfs_dbto fix these otherwise. Since we *didn't* list these extra file attributes as supported, it begs the question if instead we should only set them *and* this mask if !S_ISLNK(inode->i_mode)).Quoted 5 lines
Quoted 3 lines
If so, that also begs the question if we should add further sanity check on the xfs setattr to ensure these are never set on symbolic links, despite the fact that
FS_IOC_FSSETXATTRioctl won't be able to set them...Sure.
Alright, I'll let things wind down and if I don't hear otherwise back will make the change.
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
Quoted 41 lines · Luis R. Rodriguez
statx(2) notes that any attribute that is not indicated as supported by
stx_attributes_maskhas no usable value. Commit5f955f26f3d42d("xfs: report crtime and attribute flags to statx") added support for informing userspace of extra file attributes but forgot to list these flags as supported making reporting them rather useless for the pedantic userspace author.$ git describe --contains 5f955f26f3d42d04aba65590a32eb70eedb7f37d v4.11-rc6~5^2^2~2Fixes 5f955f26f3d42d ("xfs: report crtime and attribute flags to statx")Signed-off-by Luis R. Rodriguez <[email protected]>---
Its unclear why David left these out or if it was just a mistake, I checked the original patch thread [0] but it was not clear in the end. David?
Also, I posted a patch to add support to clearing these flags through
xfs_repairon symlinks [1] due to the pain this can cause as you have no option but to usexfs_dbto fix these otherwise. Since we *didn't* list these extra file attributes as supported, it begs the question if instead we should only set them *and* this mask if !S_ISLNK(inode->i_mode)).If so, that also begs the question if we should add further sanity check on the xfs setattr to ensure these are never set on symbolic links, despite the fact that
FS_IOC_FSSETXATTRioctl won't be able to set them...A long shot question in terms of semantics is if all the above is rather generic for Linux, is if the VFS should even be checking for immutable or append flags on unlink for symbolic links.
[0] https://patchwork.kernel.org/patch/9607879/
[1] https://lkml.kernel.org/r/[email protected]fs/xfs/xfs_iops.c | 4 ++++ 1 file changed, 4 insertions(+)fs/xfs/xfs_iops.c +0
-0
fs/xfs/xfs_iops.c xfs_vn_getattr( · line 537
Quoted 7 lines
fs/xfs/xfs_iops.c +4
-0@@ -531,6 +531,10 @@ xfs_vn_getattr(if (ip->i_d.di_flags &XFS_DIFLAG_NODUMP ) stat->attributes |=STATX_ATTR_NODUMP ; + stat->attributes_mask |= (STATX_ATTR_IMMUTABLE | +STATX_ATTR_APPEND | +STATX_ATTR_NODUMP ); +
Heh, I just posted the same diff as a patch, having forgotten that this existed. I'll ... just take this one instead.
Sorry for whiffing on this for a year and a half(!!)
--D
Quoted 10 lines
fs/xfs/xfs_iops.c +0
-0switch (inode->i_mode &S_IFMT ) {case S_IFBLK :case S_IFCHR :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