* [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes @ 2013-07-17 15:47 Dwight Engen 2013-07-19 6:02 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Dwight Engen @ 2013-07-17 15:47 UTC (permalink / raw) To: xfs Signed-off-by: Dwight Engen <dwight.engen@oracle.com> --- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 2 ++ 3 files changed, 7 insertions(+) diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h index 7eb4a5e..aee4b12 100644 --- a/fs/xfs/xfs_fs.h +++ b/fs/xfs/xfs_fs.h @@ -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 | \ diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index d873ab9e..728283a 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -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) diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index abbbdcf..e63e359 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -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; } -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes 2013-07-17 15:47 [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes Dwight Engen @ 2013-07-19 6:02 ` Dave Chinner 2013-07-19 16:13 ` Dwight Engen 2013-07-22 16:00 ` Dwight Engen 0 siblings, 2 replies; 6+ messages in thread From: Dave Chinner @ 2013-07-19 6:02 UTC (permalink / raw) To: Dwight Engen; +Cc: xfs On Wed, Jul 17, 2013 at 11:47:46AM -0400, Dwight Engen wrote: > Signed-off-by: Dwight Engen <dwight.engen@oracle.com> What's the reason for this patch? > --- > fs/xfs/xfs_fs.h | 1 + > fs/xfs/xfs_icache.c | 4 ++++ > fs/xfs/xfs_ioctl.c | 2 ++ > 3 files changed, 7 insertions(+) > > diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h > index 7eb4a5e..aee4b12 100644 > --- a/fs/xfs/xfs_fs.h > +++ b/fs/xfs/xfs_fs.h > @@ -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 | \ > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index d873ab9e..728283a 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -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) && > /* 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? > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index abbbdcf..e63e359 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes 2013-07-19 6:02 ` Dave Chinner @ 2013-07-19 16:13 ` Dwight Engen 2013-07-24 3:40 ` Dave Chinner 2013-07-22 16:00 ` Dwight Engen 1 sibling, 1 reply; 6+ messages in thread From: Dwight Engen @ 2013-07-19 16:13 UTC (permalink / raw) To: Dave Chinner, Brian Foster; +Cc: xfs On Fri, 19 Jul 2013 16:02:21 +1000 Dave Chinner <david@fromorbit.com> wrote: > On Wed, Jul 17, 2013 at 11:47:46AM -0400, Dwight Engen wrote: > > Signed-off-by: Dwight Engen <dwight.engen@oracle.com> > > 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. > > --- > > fs/xfs/xfs_fs.h | 1 + > > fs/xfs/xfs_icache.c | 4 ++++ > > fs/xfs/xfs_ioctl.c | 2 ++ > > 3 files changed, 7 insertions(+) > > > > diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h > > index 7eb4a5e..aee4b12 100644 > > --- a/fs/xfs/xfs_fs.h > > +++ b/fs/xfs/xfs_fs.h > > @@ -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 | \ > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > > index d873ab9e..728283a 100644 > > --- a/fs/xfs/xfs_icache.c > > +++ b/fs/xfs/xfs_icache.c > > @@ -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... 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)? > 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. > if ((eofb->eof_flags & XFS_EOF_FLAGS_PERM_CHECK) && > > > /* 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? Yep, I'll update those too. > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > index abbbdcf..e63e359 100644 > > --- a/fs/xfs/xfs_ioctl.c > > +++ b/fs/xfs/xfs_ioctl.c > > @@ -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. inode_permission() would catch that but I agree there is no point waiting till then to find out. > Cheers, > > Dave. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes 2013-07-19 16:13 ` Dwight Engen @ 2013-07-24 3:40 ` Dave Chinner 0 siblings, 0 replies; 6+ messages in thread From: Dave Chinner @ 2013-07-24 3:40 UTC (permalink / raw) To: Dwight Engen; +Cc: Brian Foster, xfs On Fri, Jul 19, 2013 at 12:13:21PM -0400, Dwight Engen wrote: > On Fri, 19 Jul 2013 16:02:21 +1000 > Dave Chinner <david@fromorbit.com> wrote: > > > On Wed, Jul 17, 2013 at 11:47:46AM -0400, Dwight Engen wrote: > > > Signed-off-by: Dwight Engen <dwight.engen@oracle.com> > > > > 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. background needs to be in the commit message. > > > > --- > > > fs/xfs/xfs_fs.h | 1 + > > > fs/xfs/xfs_icache.c | 4 ++++ > > > fs/xfs/xfs_ioctl.c | 2 ++ > > > 3 files changed, 7 insertions(+) > > > > > > diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h > > > index 7eb4a5e..aee4b12 100644 > > > --- a/fs/xfs/xfs_fs.h > > > +++ b/fs/xfs/xfs_fs.h > > > @@ -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 | \ > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > > > index d873ab9e..728283a 100644 > > > --- a/fs/xfs/xfs_icache.c > > > +++ b/fs/xfs/xfs_icache.c > > > @@ -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... > > 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? > > 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes 2013-07-19 6:02 ` Dave Chinner 2013-07-19 16:13 ` Dwight Engen @ 2013-07-22 16:00 ` Dwight Engen 2013-07-24 3:46 ` Dave Chinner 1 sibling, 1 reply; 6+ messages in thread From: Dwight Engen @ 2013-07-22 16:00 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On Fri, 19 Jul 2013 16:02:21 +1000 Dave Chinner <david@fromorbit.com> wrote: [...] > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > > index d873ab9e..728283a 100644 > > --- a/fs/xfs/xfs_icache.c > > +++ b/fs/xfs/xfs_icache.c > > @@ -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 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes 2013-07-22 16:00 ` Dwight Engen @ 2013-07-24 3:46 ` Dave Chinner 0 siblings, 0 replies; 6+ messages in thread From: Dave Chinner @ 2013-07-24 3:46 UTC (permalink / raw) To: Dwight Engen; +Cc: xfs On Mon, Jul 22, 2013 at 12:00:07PM -0400, Dwight Engen wrote: > On Fri, 19 Jul 2013 16:02:21 +1000 > Dave Chinner <david@fromorbit.com> wrote: > > [...] > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > > > index d873ab9e..728283a 100644 > > > --- a/fs/xfs/xfs_icache.c > > > +++ b/fs/xfs/xfs_icache.c > > > @@ -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 > > 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. > 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. 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-24 3:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-17 15:47 [PATCH v4 6/7] xfs: check that eofblocks ioctl caller can write matched inodes Dwight Engen 2013-07-19 6:02 ` Dave Chinner 2013-07-19 16:13 ` Dwight Engen 2013-07-24 3:40 ` Dave Chinner 2013-07-22 16:00 ` Dwight Engen 2013-07-24 3:46 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox