[RFC PATCH 0/2] issues with NFS filesystems as lower layer

[RFC PATCH 0/2] issues with NFS filesystems as lower layer

[Andy Whitcroft]

During some testing here we discovered that we could not successfully
use a NFS as the lower layer for overlayfs.  There are two separate issues:

Firstly when using an NFSv4 lower layer we tickle an issue when copying
up the xattrs for the underlying file.  NFS uses an xattr system.nfs4_acl
which the upper layer will not store (ext4 for example).  This triggers
an EOPNOTSUPP error when trying to copy up the xattrs for the file,
preventing the file being written.  I am a little unclear as to whether it
makes sense to generally ignore xattrs we cannot store in the upper layer,
this is based on the assumption the person creating the mount knew what
they were combining.  The first patch (for discussion) following this
email avoids this issue by ignoring the xattr if it is not storable.

Secondly when using an NFSv3 R/O lower layer the filesystem permissions
check refuses permission to write to the inode which prevents us from
copying it up even though we have a writable upper layer.  (With an ext4
lower layer the inode check will succeed if the inode  is writable even
if the filesystem is not.)  It is not clear what the right solution is
here.  One approach is to check the inode permissions only (avoiding the
filesystem specific permissions op), but it is not clear we can rely on
these for all underlying filesystems.  Perhaps this check should only be
used for NFS.  Perhaps it needs to be a mount option.  The second patch
(for discussion) following this email implements this, using the inode
permissions when the lowerlayer is read-only.  This seems to work as
expected in my limited testing.

Comments on both approaches appreciated.

-apw


Andy Whitcroft (2):
  ovl: copy_up_xattr may fail when the upper filesystem does not
    support the same xattrs
  overlayfs: when the underlying filesystem is read-only use inode
    permissions

 fs/namei.c             |   36 ++++++++++++++++++++++++++++++++++++
 fs/overlayfs/copy_up.c |    9 ++++++++-
 fs/overlayfs/inode.c   |   12 ++++++++++++
 include/linux/fs.h     |    1 +
 4 files changed, 57 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[RFC PATCH 1/2] ovl: ovl_copy_up_xattr may fail when the upper filesystem does not support the same xattrs

[Andy Whitcroft]

When the upper filesystem does not support the same extended attribute
families as the lower filesystem attempts to copy these xattrs up will
fail EOPNOTSUPP.  This is tickled by NFS which stores access information
in an NFS specific prefix.  Ignore failures to set these attributes.

BugLink: http://bugs.launchpad.net/bugs/1039402
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/overlayfs/copy_up.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 87dbeee..b0b4229 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -58,7 +58,14 @@ static int ovl_copy_up_xattr(struct dentry *old, struct dentry *new)
 			goto out_free_value;
 		}
 		error = vfs_setxattr(new, name, value, size, 0);
-		if (error)
+		/*
+		 * It is possible that the upper filesystem does not
+                 * support the same xattrs as the lower filesystem.  Ignore
+                 * unsupported types.
+                 */
+		if (error == -EOPNOTSUPP)
+			error = 0;
+		else if (error)
 			goto out_free_value;
 	}
 
-- 
1.7.9.5

[PATCH 2/2] overlayfs: when the underlying filesystem is read-only use inode permissions

[Andy Whitcroft]

When the lowerdir represents a read-only filesystems, attempts to write
to the overlay will check for write permissions on files in the lower
layer via the lower layer inode .permissions operation.  This operation
may take the read-only status into account and fail the request even
though the notional permissions on the file permit write.  This prevents
a copy_up occuring on the file and fails the write.

Switch to using inode permissions directly when the lower layer is
read-only.

BugLink: http://bugs.launchpad.net/bugs/1039402
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/namei.c           |   36 ++++++++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c |   12 ++++++++++++
 include/linux/fs.h   |    1 +
 3 files changed, 49 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 9be439a..09925a9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -315,6 +315,42 @@ static inline int do_inode_permission(struct inode *inode, int mask)
 }
 
 /**
+ * __inode_permission_generic - Check for access rights to a given inode
+ * @inode: Inode to check permission on
+ * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
+ *
+ * Check for read/write/execute permissions on an inode.
+ *
+ * When checking for MAY_APPEND, MAY_WRITE must also be set in @mask.
+ *
+ * This does not check for a read-only file system.  You probably want
+ * inode_permission().
+ */
+int __inode_permission_generic(struct inode *inode, int mask)
+{
+	int retval;
+
+	if (unlikely(mask & MAY_WRITE)) {
+		/*
+		 * Nobody gets write access to an immutable file.
+		 */
+		if (IS_IMMUTABLE(inode))
+			return -EACCES;
+	}
+
+	retval = generic_permission(inode, mask);
+	if (retval)
+		return retval;
+
+	retval = devcgroup_inode_permission(inode, mask);
+	if (retval)
+		return retval;
+
+	return security_inode_permission(inode, mask);
+}
+EXPORT_SYMBOL(__inode_permission_generic);
+
+/**
  * __inode_permission - Check for access rights to a given inode
  * @inode: Inode to check permission on
  * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index e7ab09b..00390f2 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -102,6 +102,18 @@ int ovl_permission(struct inode *inode, int mask)
 		if (is_upper && !IS_RDONLY(inode) && IS_RDONLY(realinode) &&
 		    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
 			goto out_dput;
+
+		/*
+		 * If the lower layer is read-only the filesystem may
+		 * fail permissions checks for write even if the file
+		 * itself if writable and in a overlay would be eligable
+		 * for copy up.  Use the inode permission in this case
+		 * via generic_permission().
+		 */
+		if (!is_upper && !IS_RDONLY(inode) && IS_RDONLY(realinode)) {
+			err = __inode_permission_generic(realinode, mask);
+			goto out_dput;
+		}
 	}
 
 	err = __inode_permission(realinode, mask);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d573703..5e72ba7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2419,6 +2419,7 @@ extern sector_t bmap(struct inode *, sector_t);
 extern int notify_change(struct dentry *, struct iattr *);
 extern int inode_permission(struct inode *, int);
 extern int __inode_permission(struct inode *, int);
+extern int __inode_permission_generic(struct inode *, int);
 extern int generic_permission(struct inode *, int);
 
 static inline bool execute_ok(struct inode *inode)
-- 
1.7.9.5

Re: [RFC PATCH 0/2] issues with NFS filesystems as lower layer

[Miklos Szeredi]

On Thu, Sep 6, 2012 at 5:56 PM, Andy Whitcroft <apw@canonical.com> wrote:
> During some testing here we discovered that we could not successfully
> use a NFS as the lower layer for overlayfs.  There are two separate issues:
>
> Firstly when using an NFSv4 lower layer we tickle an issue when copying
> up the xattrs for the underlying file.  NFS uses an xattr system.nfs4_acl
> which the upper layer will not store (ext4 for example).  This triggers
> an EOPNOTSUPP error when trying to copy up the xattrs for the file,
> preventing the file being written.  I am a little unclear as to whether it
> makes sense to generally ignore xattrs we cannot store in the upper layer,
> this is based on the assumption the person creating the mount knew what
> they were combining.  The first patch (for discussion) following this
> email avoids this issue by ignoring the xattr if it is not storable.

I don't know much about NFSv4 ACL's but I think it may be incompatible
with POSIX ACLs in which case copying them up is not possible and
ignoring them should be the right thing to do.

>
> Secondly when using an NFSv3 R/O lower layer the filesystem permissions
> check refuses permission to write to the inode which prevents us from
> copying it up even though we have a writable upper layer.  (With an ext4
> lower layer the inode check will succeed if the inode  is writable even
> if the filesystem is not.)  It is not clear what the right solution is
> here.  One approach is to check the inode permissions only (avoiding the
> filesystem specific permissions op), but it is not clear we can rely on
> these for all underlying filesystems.  Perhaps this check should only be
> used for NFS.  Perhaps it needs to be a mount option.  The second patch
> (for discussion) following this email implements this, using the inode
> permissions when the lowerlayer is read-only.  This seems to work as
> expected in my limited testing.

I fear that will create an inconsistency between the read-only and the
non-read-only case, even though both should behave the same.

I think the cleanest would be to create a mount option to always use
generic_permission (on both the lower and the upper fs).  That would
give us two, slightly different, operating modes but each would be
self consistent.

Thanks,
Miklos

Re: [RFC PATCH 0/2] issues with NFS filesystems as lower layer

[J. Bruce Fields]

On Fri, Sep 07, 2012 at 08:35:36AM +0200, Miklos Szeredi wrote:
> On Thu, Sep 6, 2012 at 5:56 PM, Andy Whitcroft <apw@canonical.com> wrote:
> > During some testing here we discovered that we could not successfully
> > use a NFS as the lower layer for overlayfs.  There are two separate issues:
> >
> > Firstly when using an NFSv4 lower layer we tickle an issue when copying
> > up the xattrs for the underlying file.  NFS uses an xattr system.nfs4_acl
> > which the upper layer will not store (ext4 for example).  This triggers
> > an EOPNOTSUPP error when trying to copy up the xattrs for the file,
> > preventing the file being written.  I am a little unclear as to whether it
> > makes sense to generally ignore xattrs we cannot store in the upper layer,
> > this is based on the assumption the person creating the mount knew what
> > they were combining.  The first patch (for discussion) following this
> > email avoids this issue by ignoring the xattr if it is not storable.
> 
> I don't know much about NFSv4 ACL's but I think it may be incompatible
> with POSIX ACLs in which case copying them up is not possible and

Right.  (You can try to map them; see fs/nfsd/nfs4acl.c; but it's
complicated and lossy.)

> ignoring them should be the right thing to do.

The ACLs are enforced by the server side, so this won't let you read or
write server data that you couldn't before.

And you also shouldn't be able to access the copied-up file in ways you
couldn't before as long as the lower filesystem is consulted about
permissions.

> > Secondly when using an NFSv3 R/O lower layer the filesystem permissions
> > check refuses permission to write to the inode which prevents us from
> > copying it up even though we have a writable upper layer.  (With an ext4
> > lower layer the inode check will succeed if the inode  is writable even
> > if the filesystem is not.)  It is not clear what the right solution is
> > here.  One approach is to check the inode permissions only (avoiding the
> > filesystem specific permissions op), but it is not clear we can rely on
> > these for all underlying filesystems.  Perhaps this check should only be
> > used for NFS.

Then couldn't you for example end up circumventing ACLs on the
underlying file to access data cached by reads from another user on the
same system?

Is it possible to arrange that the check for a readonly filesystem be
done only by the vfs and not also by ->permission?

--b.

> > Perhaps it needs to be a mount option.  The second patch
> > (for discussion) following this email implements this, using the inode
> > permissions when the lowerlayer is read-only.  This seems to work as
> > expected in my limited testing.
> 
> I fear that will create an inconsistency between the read-only and the
> non-read-only case, even though both should behave the same.
> 
> I think the cleanest would be to create a mount option to always use
> generic_permission (on both the lower and the upper fs).  That would
> give us two, slightly different, operating modes but each would be
> self consistent.
> 
> Thanks,
> Miklos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] issues with NFS filesystems as lower layer

[Miklos Szeredi]

"J. Bruce Fields" <bfields@fieldses.org> writes:

>> > Secondly when using an NFSv3 R/O lower layer the filesystem permissions
>> > check refuses permission to write to the inode which prevents us from
>> > copying it up even though we have a writable upper layer.  (With an ext4
>> > lower layer the inode check will succeed if the inode  is writable even
>> > if the filesystem is not.)  It is not clear what the right solution is
>> > here.  One approach is to check the inode permissions only (avoiding the
>> > filesystem specific permissions op), but it is not clear we can rely on
>> > these for all underlying filesystems.  Perhaps this check should only be
>> > used for NFS.
>
> Then couldn't you for example end up circumventing ACLs on the
> underlying file to access data cached by reads from another user on the
> same system?

Ignoring ACL's should always give less access, isn't that right?

>
> Is it possible to arrange that the check for a readonly filesystem be
> done only by the vfs and not also by ->permission?

You'd need to modify NFS servers for that to work, no?  It's possible
but not practical.

Thanks,
Miklos



>
> --b.
>
>> > Perhaps it needs to be a mount option.  The second patch
>> > (for discussion) following this email implements this, using the inode
>> > permissions when the lowerlayer is read-only.  This seems to work as
>> > expected in my limited testing.
>> 
>> I fear that will create an inconsistency between the read-only and the
>> non-read-only case, even though both should behave the same.
>> 
>> I think the cleanest would be to create a mount option to always use
>> generic_permission (on both the lower and the upper fs).  That would
>> give us two, slightly different, operating modes but each would be
>> self consistent.
>> 
>> Thanks,
>> Miklos
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] issues with NFS filesystems as lower layer

[J. Bruce Fields]

On Tue, Sep 11, 2012 at 10:56:52PM +0200, Miklos Szeredi wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> >> > Secondly when using an NFSv3 R/O lower layer the filesystem permissions
> >> > check refuses permission to write to the inode which prevents us from
> >> > copying it up even though we have a writable upper layer.  (With an ext4
> >> > lower layer the inode check will succeed if the inode  is writable even
> >> > if the filesystem is not.)  It is not clear what the right solution is
> >> > here.  One approach is to check the inode permissions only (avoiding the
> >> > filesystem specific permissions op), but it is not clear we can rely on
> >> > these for all underlying filesystems.  Perhaps this check should only be
> >> > used for NFS.
> >
> > Then couldn't you for example end up circumventing ACLs on the
> > underlying file to access data cached by reads from another user on the
> > same system?
> 
> Ignoring ACL's should always give less access, isn't that right?

Not necessarily.

(It's up to the server--and if anything servers probably want to err on
the side of returning mode bits that are an upper, not a lower, bound on
the permissions.)

> > Is it possible to arrange that the check for a readonly filesystem be
> > done only by the vfs and not also by ->permission?
> 
> You'd need to modify NFS servers for that to work, no?  It's possible
> but not practical.

Oh, OK, I guess I assumed you were dealing with an NFS filesystem that
had been mounted readonly on the NFS client.

If it's a read-write mount of a filesystem that's read-only on the
server side: well, there is at least an error for that case: the server
should return NFSERR_ROFS, and you should see EROFS--could you do the
copy-up only in the case you get that error?

--b.

> 
> Thanks,
> Miklos
> 
> 
> 
> >
> > --b.
> >
> >> > Perhaps it needs to be a mount option.  The second patch
> >> > (for discussion) following this email implements this, using the inode
> >> > permissions when the lowerlayer is read-only.  This seems to work as
> >> > expected in my limited testing.
> >> 
> >> I fear that will create an inconsistency between the read-only and the
> >> non-read-only case, even though both should behave the same.
> >> 
> >> I think the cleanest would be to create a mount option to always use
> >> generic_permission (on both the lower and the upper fs).  That would
> >> give us two, slightly different, operating modes but each would be
> >> self consistent.
> >> 
> >> Thanks,
> >> Miklos
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] issues with NFS filesystems as lower layer

[Miklos Szeredi]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Tue, Sep 11, 2012 at 10:56:52PM +0200, Miklos Szeredi wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> 
>> >> > Secondly when using an NFSv3 R/O lower layer the filesystem permissions
>> >> > check refuses permission to write to the inode which prevents us from
>> >> > copying it up even though we have a writable upper layer.  (With an ext4
>> >> > lower layer the inode check will succeed if the inode  is writable even
>> >> > if the filesystem is not.)  It is not clear what the right solution is
>> >> > here.  One approach is to check the inode permissions only (avoiding the
>> >> > filesystem specific permissions op), but it is not clear we can rely on
>> >> > these for all underlying filesystems.  Perhaps this check should only be
>> >> > used for NFS.
>> >
>> > Then couldn't you for example end up circumventing ACLs on the
>> > underlying file to access data cached by reads from another user on the
>> > same system?
>> 
>> Ignoring ACL's should always give less access, isn't that right?
>
> Not necessarily.
>
> (It's up to the server--and if anything servers probably want to err on
> the side of returning mode bits that are an upper, not a lower, bound on
> the permissions.)

Okay, I looked at the POSIX ACL access check algorithm and now
understand things better.

Now i think that enforcing ACL's on the overlay is not possible if ACL's
are not copied up together with the data.

This is because during the copy-up the server checks for access, but it
checks access for only a single user/group pair.  Subsequent access
checks on the copied-up object by any other user/group may not reflect
the original ACL's on the lower filesystem.

So assuming that we can't copy up NFS ACL's, it is possible to
circumvent those ACL's by triggering a copy-up.  Of course this type of
"attack" is limited by the fact that copy-up is only triggerable by a
write type operation.

I'm not sure...  I believe that we should say that NFS ACL's on the
lower layer and overlayfs simply don't mix.  In that case current
behavior of erroring out seems to be the correct one.

Andy, I'm wondering: what is your use case for NFS with ACL's?

Because overlayfs can't completely ignore ACL's on the lower filesystem,
but it can't enforce them properly either.  So it's a rather weird
situation.


>
>> > Is it possible to arrange that the check for a readonly filesystem be
>> > done only by the vfs and not also by ->permission?
>> 
>> You'd need to modify NFS servers for that to work, no?  It's possible
>> but not practical.
>
> Oh, OK, I guess I assumed you were dealing with an NFS filesystem that
> had been mounted readonly on the NFS client.
>
> If it's a read-write mount of a filesystem that's read-only on the
> server side: well, there is at least an error for that case: the server
> should return NFSERR_ROFS, and you should see EROFS--could you do the
> copy-up only in the case you get that error?

If the server returns EROFS then all you know is that the filesystem is
read-only.  But that's not what we are interested in.  We are interested
in whether the access would be allowed *if* the filesystem *wasn't*
read-only.  The server doesn't tell us that.

Thanks,
Miklos

Re: [RFC PATCH 0/2] issues with NFS filesystems as lower layer

[J. Bruce Fields]

On Wed, Sep 12, 2012 at 05:20:17PM +0200, Miklos Szeredi wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > On Tue, Sep 11, 2012 at 10:56:52PM +0200, Miklos Szeredi wrote:
> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >> 
> >> >> > Secondly when using an NFSv3 R/O lower layer the filesystem permissions
> >> >> > check refuses permission to write to the inode which prevents us from
> >> >> > copying it up even though we have a writable upper layer.  (With an ext4
> >> >> > lower layer the inode check will succeed if the inode  is writable even
> >> >> > if the filesystem is not.)  It is not clear what the right solution is
> >> >> > here.  One approach is to check the inode permissions only (avoiding the
> >> >> > filesystem specific permissions op), but it is not clear we can rely on
> >> >> > these for all underlying filesystems.  Perhaps this check should only be
> >> >> > used for NFS.
> >> >
> >> > Then couldn't you for example end up circumventing ACLs on the
> >> > underlying file to access data cached by reads from another user on the
> >> > same system?
> >> 
> >> Ignoring ACL's should always give less access, isn't that right?
> >
> > Not necessarily.
> >
> > (It's up to the server--and if anything servers probably want to err on
> > the side of returning mode bits that are an upper, not a lower, bound on
> > the permissions.)
> 
> Okay, I looked at the POSIX ACL access check algorithm and now
> understand things better.
> 
> Now i think that enforcing ACL's on the overlay is not possible if ACL's
> are not copied up together with the data.

And the NFSv4 case is different (it doesn't use "posix" acls), etc.,
etc.

But forget all that, even mode bits aren't much use since you don't
understand how the server might be mapping your uid, so you don't know
whether you're the owner, or a member of the group.

On the other hand, for some cases maybe what you want is to just forget
all that and trust the file owners and mode bits, so maybe that's useful
at least as an option.

> > Oh, OK, I guess I assumed you were dealing with an NFS filesystem that
> > had been mounted readonly on the NFS client.
> >
> > If it's a read-write mount of a filesystem that's read-only on the
> > server side: well, there is at least an error for that case: the server
> > should return NFSERR_ROFS, and you should see EROFS--could you do the
> > copy-up only in the case you get that error?
> 
> If the server returns EROFS then all you know is that the filesystem is
> read-only.  But that's not what we are interested in.  We are interested
> in whether the access would be allowed *if* the filesystem *wasn't*
> read-only.  The server doesn't tell us that.

Yeah.  I wonder if it would be reasonable in the future to let servers
tell us that.

--b.