On Wed, Jul 17, 2013 at 11:47:46AM -0400, Dwight Engen wrote: > Signed-off-by: Dwight Engen <dwight.engen@xxxxxxxxxx> 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@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs