xfs: check that eofblocks ioctl caller can write matched inodes
PATCH v4 6/7 xfs: check that eofblocks ioctl caller can write matched inodes
--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 2 ++ 3 files changed, 7 insertions(+)
fs/xfs/xfs_fs.h +1 -0
@@ -361,6 +361,7 @@ struct xfs_fs_eofblocks {#define XFS_EOF_FLAGS_GID (1 << 2)/* filter by gid */ #define XFS_EOF_FLAGS_PRID (1 << 3)/* filter by project id */ #define XFS_EOF_FLAGS_MINFILESIZE (1 << 4)/* filter by min file size */ +#define XFS_EOF_FLAGS_PERM_CHECK (1 << 5)/* check can write inode */ #define XFS_EOF_FLAGS_VALID \ (XFS_EOF_FLAGS_SYNC | \ XFS_EOF_FLAGS_UID | \
fs/xfs/xfs_icache.c +4 -0
@@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(if (!xfs_inode_match_id (ip, eofb))return 0 ; +if (eofb->eof_flags &XFS_EOF_FLAGS_PERM_CHECK && +inode_permission (VFS_I (ip),MAY_WRITE )) +return 0 ; +/* skip the inode if the file size is too small */ if (eofb->eof_flags &XFS_EOF_FLAGS_MINFILESIZE &&XFS_ISIZE (ip) < eofb->eof_min_file_size)
fs/xfs/xfs_ioctl.c +2 -0
@@ -1636,6 +1636,8 @@ xfs_file_ioctl( !gid_valid(keofb.eof_gid))return XFS_ERROR (EINVAL ); + keofb.eof_flags |=XFS_EOF_FLAGS_PERM_CHECK ; + error =xfs_icache_free_eofblocks (mp, &keofb);return -error; }
Signature · 6 lines
1.8.1.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Dwight Engen
Signed-off-by Dwight Engen <[email protected]>
What's the reason for this patch?
Quoted 23 lines
--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 2 ++ 3 files changed, 7 insertions(+)fs/xfs/xfs_fs.h +1
-0@@ -361,6 +361,7 @@ struct xfs_fs_eofblocks {#define XFS_EOF_FLAGS_GID (1 << 2)/* filter by gid */ #define XFS_EOF_FLAGS_PRID (1 << 3)/* filter by project id */ #define XFS_EOF_FLAGS_MINFILESIZE (1 << 4)/* filter by min file size */ +#define XFS_EOF_FLAGS_PERM_CHECK (1 << 5)/* check can write inode */ #define XFS_EOF_FLAGS_VALID \ (XFS_EOF_FLAGS_SYNC | \ XFS_EOF_FLAGS_UID | \fs/xfs/xfs_icache.c +0
-0
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1252
Quoted 6 lines
fs/xfs/xfs_icache.c +3
-0@@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(if (!xfs_inode_match_id (ip, eofb))return 0 ; +if (eofb->eof_flags &XFS_EOF_FLAGS_PERM_CHECK && +inode_permission (VFS_I (ip),MAY_WRITE )) +return 0 ;
This assumes we are walking fully instantiated VFS inodes. That's not necessarily true - we may be walking inodes that have already been dropped from the VFS and are waiting for background reclaim to clean them up. I suspect that this doesn't need to be done - we normally stop background modification processes like this when we convert the filesystem to read-only. I suspect the eof-blocks scan code is missing that, and so it can potentially run on a RO filesystem. That needs fixing similar to the way we stop and start the periodic log work...
Also, gcc should throw warnings on that code (strange, it didn't here on gcc-4.7) as it needs more parenthesis. i.e
if ((eofb->eof_flags & XFS_EOF_FLAGS_PERM_CHECK) &&
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1256
Quoted 3 lines
fs/xfs/xfs_icache.c +0
-0/* skip the inode if the file size is too small */ if (eofb->eof_flags &XFS_EOF_FLAGS_MINFILESIZE &&XFS_ISIZE (ip) < eofb->eof_min_file_size)
Oh, I see you are just copying other code. How did I miss that in a past review? :(
Hmmm - it looks like there's a bunch of them in xfs_inode_match_id() as well, and you touched all those if() statements in a previous patch. can you go back to the patch that touches xfs_inode_match_id() and add the extra () there as well?
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1639
Quoted 9 lines
fs/xfs/xfs_ioctl.c +1
-0@@ -1636,6 +1636,8 @@ xfs_file_ioctl( !gid_valid(keofb.eof_gid))return XFS_ERROR (EINVAL ); + keofb.eof_flags |=XFS_EOF_FLAGS_PERM_CHECK ;
We should be checking for the fs being RO here and aborting if it is.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 4 lines · On Fri, 19 Jul 2013 16:02:21 +1000 Dave Chinner <[email protected]> wrote:
Quoted 1 line · Dwight Engen
Signed-off-by Dwight Engen <[email protected]>What's the reason for this patch?
Its trying to ensure we only allow the XFS_IOC_FREE_EOFBLOCKS caller to affect the indoes they should be able to.
http://oss.sgi.com/archives/xfs/2013-06/msg00955.html has a bit more background. This isn't really related to user namespaces per-se, so I guess it should be a separate patch, but since I modified the eofblocks structure I was trying to fix this as well.
Quoted 26 lines
Quoted 26 lines
--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 2 ++ 3 files changed, 7 insertions(+)fs/xfs/xfs_fs.h +0
-0@@ -361,6 +361,7 @@ struct xfs_fs_eofblocks {#define XFS_EOF_FLAGS_GID (1 << 2)/* filter by gid */ #define XFS_EOF_FLAGS_PRID (1 << 3)/* filter by project id */ #define
XFS_EOF_FLAGS_MINFILESIZE(1 << 4) /*
filter by min file size */ +#defineXFS_EOF_FLAGS_PERM_CHECK(1 << 5) /* check can write inode */ #defineXFS_EOF_FLAGS_VALID\ (XFS_EOF_FLAGS_SYNC| \fs/xfs/xfs_fs.h +0
-0XFS_EOF_FLAGS_UID | \fs/xfs/xfs_icache.c +0
-0@@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(if (!xfs_inode_match_id (ip, eofb))return 0 ;
Quoted 13 lines
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1252
Quoted 3 lines
fs/xfs/xfs_icache.c +3
-0+if (eofb->eof_flags &XFS_EOF_FLAGS_PERM_CHECK && +inode_permission (VFS_I (ip),MAY_WRITE )) +return 0 ;This assumes we are walking fully instantiated VFS inodes. That's not necessarily true - we may be walking inodes that have already been dropped from the VFS and are waiting for background reclaim to clean them up. I suspect that this doesn't need to be done - we normally stop background modification processes like this when we convert the filesystem to read-only. I suspect the eof-blocks scan code is missing that, and so it can potentially run on a RO filesystem. That needs fixing similar to the way we stop and start the periodic log work...
So if there isn't a good way to check per-inode, maybe for now we should just restrict the ioctl caller to be capable(CAP_SYS_ADMIN)?
Quoted 2 lines
Also, gcc should throw warnings on that code (strange, it didn't here on gcc-4.7) as it needs more parenthesis. i.e
I don't think it needs them (& is higher precedence than &&), but I can add them for clarity if you like.
Quoted 6 lines
if ((eofb->eof_flags & XFS_EOF_FLAGS_PERM_CHECK) &&fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1256
Quoted 3 lines
fs/xfs/xfs_icache.c +0
-0/* skip the inode if the file size is too small */ if (eofb->eof_flags &XFS_EOF_FLAGS_MINFILESIZE &&XFS_ISIZE (ip) < eofb->eof_min_file_size)
Quoted 7 lines
Oh, I see you are just copying other code. How did I miss that in a past review? :(
Hmmm - it looks like there's a bunch of them in
xfs_inode_match_id()as well, and you touched all thoseif()statements in a previous patch. can you go back to the patch that touchesxfs_inode_match_id()and add the extra () there as well?
Yep, I'll update those too.
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_ioctl.c +0
-0@@ -1636,6 +1636,8 @@ xfs_file_ioctl( !gid_valid(keofb.eof_gid))return XFS_ERROR (EINVAL );
Quoted 4 lines
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1639
Quoted 1 line
fs/xfs/xfs_ioctl.c +1
-0+ keofb.eof_flags |=XFS_EOF_FLAGS_PERM_CHECK ;We should be checking for the fs being RO here and aborting if it is.
inode_permission() would catch that but I agree there is no point waiting till then to find out.
Quoted 3 lines
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
On Fri, 19 Jul 2013 16:02:21 +1000
Dave Chinner <[email protected]> wrote:
[...]
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_icache.c +0
-0@@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(if (!xfs_inode_match_id (ip, eofb))return 0 ;
Quoted 7 lines
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1252
Quoted 3 lines
fs/xfs/xfs_icache.c +3
-0+if (eofb->eof_flags &XFS_EOF_FLAGS_PERM_CHECK && +inode_permission (VFS_I (ip),MAY_WRITE )) +return 0 ;This assumes we are walking fully instantiated VFS inodes. That's not necessarily true - we may be walking inodes that have already been dropped from the VFS and are waiting for background reclaim to
Hi Dave, in looking at this a bit I don't see how they can be dropped from the VFS since they are igrab()ed in the flow:
xfs_icache_free_eofblocks
xfs_inode_ag_iterator_tag
xfs_inode_ag_walk
xfs_inode_ag_walk_grab
igrab
and I don't see a way for xfs_inode_free_eofblocks() to be called other than the ag_walk flow.
If there is a way to get into xfs_inode_free_eofblocks where we can't use VFS_I(ip) then it will be a problem for the new code in xfs_inode_match_id() as well.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 6 lines · Dwight Engen
Quoted 3 lines · On Fri, 19 Jul 2013 16:02:21 +1000 Dave Chinner <[email protected]> wrote:
Quoted 1 line · Dwight Engen
Signed-off-by Dwight Engen <[email protected]>
Quoted 8 lines
Quoted 1 line
What's the reason for this patch?
Its trying to ensure we only allow the
XFS_IOC_FREE_EOFBLOCKScaller to affect the indoes they should be able to.
http://oss.sgi.com/archives/xfs/2013-06/msg00955.html has a bit more background. This isn't really related to user namespaces per-se, so I guess it should be a separate patch, but since I modified the eofblocks structure I was trying to fix this as well.
background needs to be in the commit message.
Quoted 41 lines
Quoted 27 lines
Quoted 27 lines
--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 2 ++ 3 files changed, 7 insertions(+)fs/xfs/xfs_fs.h +0
-0@@ -361,6 +361,7 @@ struct xfs_fs_eofblocks {#define XFS_EOF_FLAGS_GID (1 << 2)/* filter by gid */ #define XFS_EOF_FLAGS_PRID (1 << 3)/* filter by project id */ #define
XFS_EOF_FLAGS_MINFILESIZE(1 << 4) /*
filter by min file size */ +#defineXFS_EOF_FLAGS_PERM_CHECK(1 << 5) /* check can write inode */ #defineXFS_EOF_FLAGS_VALID\ (XFS_EOF_FLAGS_SYNC| \fs/xfs/xfs_fs.h +0
-0XFS_EOF_FLAGS_UID | \fs/xfs/xfs_icache.c +0
-0@@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(if (!xfs_inode_match_id (ip, eofb))return 0 ;Quoted 13 lines
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1252
Quoted 3 lines
fs/xfs/xfs_icache.c +3
-0+if (eofb->eof_flags &XFS_EOF_FLAGS_PERM_CHECK && +inode_permission (VFS_I (ip),MAY_WRITE )) +return 0 ;This assumes we are walking fully instantiated VFS inodes. That's not necessarily true - we may be walking inodes that have already been dropped from the VFS and are waiting for background reclaim to clean them up. I suspect that this doesn't need to be done - we normally stop background modification processes like this when we convert the filesystem to read-only. I suspect the eof-blocks scan code is missing that, and so it can potentially run on a RO filesystem. That needs fixing similar to the way we stop and start the periodic log work...
Quoted 2 lines
So if there isn't a good way to check per-inode, maybe for now we should just restrict the ioctl caller to be
capable(CAP_SYS_ADMIN)?
What, exactly, are you trying to check here?
Quoted 5 lines
Quoted 2 lines
Also, gcc should throw warnings on that code (strange, it didn't here on gcc-4.7) as it needs more parenthesis. i.e
I don't think it needs them (& is higher precedence than &&), but I can add them for clarity if you like.
I know what the precedence is, but code that looks like:
(a & b && c & d && b & d && ..)
needs time to verify that it is correct. Indeed, when I see the above, I think "was it supposed to be":
(a && b && c && d && b & d && ..)
Parenthesis remove any ambiguity in intention here - they clearly separate intended logic from typos. Same goes for | vs ||....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 20 lines · Dwight Engen
On Fri, 19 Jul 2013 16:02:21 +1000
Dave Chinner <[email protected]> wrote:[...]
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_icache.c +0
-0@@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(if (!xfs_inode_match_id (ip, eofb))return 0 ;Quoted 7 lines
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1252
Quoted 3 lines
fs/xfs/xfs_icache.c +3
-0+if (eofb->eof_flags &XFS_EOF_FLAGS_PERM_CHECK && +inode_permission (VFS_I (ip),MAY_WRITE )) +return 0 ;This assumes we are walking fully instantiated VFS inodes. That's not necessarily true - we may be walking inodes that have already been dropped from the VFS and are waiting for background reclaim to
Quoted 8 lines
Hi Dave, in looking at this a bit I don't see how they can be dropped from the VFS since they are igrab()ed in the flow:
xfs_icache_free_eofblocks
xfs_inode_ag_iterator_tag
xfs_inode_ag_walk
xfs_inode_ag_walk_grab
igrab
Ah, right, I forgot that we have two different methods of walking that tree, and the one that xfs_icache_free_eofblocks() avoids inodes in reclaim.
Quoted 6 lines
and I don't see a way for
xfs_inode_free_eofblocks()to be called other than theag_walkflow.If there is a way to get into
xfs_inode_free_eofblockswhere we can't useVFS_I(ip)then it will be a problem for the new code inxfs_inode_match_id()as well.
We can always use VFS_I(ip) because the struct inode is embedded in the struct xfs_inode, so that's never an issue.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs