* [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 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-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-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