diff mbox series

OVL: add honoracl=off mount option.

Message ID 8736lx4goa.fsf@notabene.neil.brown.name (mailing list archive)
State New, archived
Headers show
Series OVL: add honoracl=off mount option. | expand

Commit Message

NeilBrown May 2, 2019, 4:35 a.m. UTC
If the upper and lower layers use incompatible ACL formats, it is not
possible to copy the ACL xttr from one to the other, so overlayfs
cannot work with them.
This happens particularly with NFSv4 which uses system.nfs4_acl, and
ext4 which uses system.posix_acl_access.

If all ACLs actually make to Unix permissions, then there is no need
to copy up the ACLs, but overlayfs cannot determine this.

So allow the sysadmin it assert that ACLs are not needed with a mount
option
  honoracl=off
This causes the ACLs to not be copied, so filesystems with different
ACL formats can be overlaid together.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
 fs/overlayfs/copy_up.c                  |  9 +++++++--
 fs/overlayfs/dir.c                      |  2 +-
 fs/overlayfs/overlayfs.h                |  2 +-
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 15 +++++++++++++++
 6 files changed, 49 insertions(+), 4 deletions(-)

Comments

Randy Dunlap May 2, 2019, 5:08 a.m. UTC | #1
Hi Neil,

On 5/1/19 9:35 PM, NeilBrown wrote:
> 
> If the upper and lower layers use incompatible ACL formats, it is not
> possible to copy the ACL xttr from one to the other, so overlayfs

                           attr (?)

> cannot work with them.
> This happens particularly with NFSv4 which uses system.nfs4_acl, and
> ext4 which uses system.posix_acl_access.
> 
> If all ACLs actually make to Unix permissions, then there is no need

                       map (?)

> to copy up the ACLs, but overlayfs cannot determine this.
> 
> So allow the sysadmin it assert that ACLs are not needed with a mount
> option
>   honoracl=off
> This causes the ACLs to not be copied, so filesystems with different
> ACL formats can be overlaid together.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
>  fs/overlayfs/copy_up.c                  |  9 +++++++--
>  fs/overlayfs/dir.c                      |  2 +-
>  fs/overlayfs/overlayfs.h                |  2 +-
>  fs/overlayfs/ovl_entry.h                |  1 +
>  fs/overlayfs/super.c                    | 15 +++++++++++++++
>  6 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index eef7d9d259e8..7ad675940c93 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
>  overlay filesystem (though an operation on the name of the file such as
>  rename or unlink will of course be noticed and handled).
>  
> +ACL copy-up
> +-----------
> +
> +When a file that only exists on the lower layer is modified it needs
> +to be copied up to the upper layer.  This means copying the metadata
> +and (usually) the data (though see "Metadata only copy up" below).
> +One part of the metadata can be problematic: the ACLs.
> +
> +Now all filesystems support ACLs, and when they do they don't all use

   Not

> +the same format.  A significant conflict appears between POSIX acls

                                                                  ACLs

> +used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There

                                                                    These (or the)

> +two formats are, in general, not inter-convertible.
> +
> +If a site only uses regular Unix permissions (Read, Write, eXecute by
> +User, Group and Other), then as these permissions are compatible with
> +all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
> +if this is the case itself.
> +
> +For this reason, overlayfs supports a mount option "honoracl=off"
> +which causes ACLs, any "system." extended attribute, on the lower
> +layer to be ignored and, particularly, not copied to the upper later.
> +This allows NFSv4 to be overlaid with a local filesystem, but should
> +only be used if the only access controls used on the filesystem are
> +Unix permission bits.
>  
>  Multiple lower layers
>  ---------------------
Amir Goldstein May 2, 2019, 11:46 a.m. UTC | #2
On Thu, May 2, 2019 at 12:35 AM NeilBrown <neilb@suse.com> wrote:
>
>
> If the upper and lower layers use incompatible ACL formats, it is not
> possible to copy the ACL xttr from one to the other, so overlayfs
> cannot work with them.
> This happens particularly with NFSv4 which uses system.nfs4_acl, and
> ext4 which uses system.posix_acl_access.
>
> If all ACLs actually make to Unix permissions, then there is no need
> to copy up the ACLs, but overlayfs cannot determine this.
>
> So allow the sysadmin it assert that ACLs are not needed with a mount
> option
>   honoracl=off
> This causes the ACLs to not be copied, so filesystems with different
> ACL formats can be overlaid together.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
>  fs/overlayfs/copy_up.c                  |  9 +++++++--
>  fs/overlayfs/dir.c                      |  2 +-
>  fs/overlayfs/overlayfs.h                |  2 +-
>  fs/overlayfs/ovl_entry.h                |  1 +
>  fs/overlayfs/super.c                    | 15 +++++++++++++++
>  6 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index eef7d9d259e8..7ad675940c93 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
>  overlay filesystem (though an operation on the name of the file such as
>  rename or unlink will of course be noticed and handled).
>
> +ACL copy-up
> +-----------
> +
> +When a file that only exists on the lower layer is modified it needs
> +to be copied up to the upper layer.  This means copying the metadata
> +and (usually) the data (though see "Metadata only copy up" below).
> +One part of the metadata can be problematic: the ACLs.
> +
> +Now all filesystems support ACLs, and when they do they don't all use
> +the same format.  A significant conflict appears between POSIX acls
> +used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There
> +two formats are, in general, not inter-convertible.
> +
> +If a site only uses regular Unix permissions (Read, Write, eXecute by
> +User, Group and Other), then as these permissions are compatible with
> +all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
> +if this is the case itself.
> +
> +For this reason, overlayfs supports a mount option "honoracl=off"
> +which causes ACLs, any "system." extended attribute, on the lower
> +layer to be ignored and, particularly, not copied to the upper later.
> +This allows NFSv4 to be overlaid with a local filesystem, but should
> +only be used if the only access controls used on the filesystem are
> +Unix permission bits.
>

I don't know. On the one hand "system." is not only ACLs.
On the other hand, "honoracl=off" is not the same as -o noacl,
but it sure sounds the same.

I'd be a lot more comfortable with "ignore_xattrs=system.nfs4_acl"
argument takes a comma separated list of xattr prefixes to ignore.

ovl_is_private_xattr() can be generalized to ovl_is_ignored_xattr(),
going over a blacklist of N>=1 which will also be called from
ovl_can_list(), because there is no point in listing the ACLs that
are ignored. right?

Thanks,
Amir.
J. R. Okajima May 2, 2019, 1:47 p.m. UTC | #3
NeilBrown:
> If the upper and lower layers use incompatible ACL formats, it is not
> possible to copy the ACL xttr from one to the other, so overlayfs
> cannot work with them.
> This happens particularly with NFSv4 which uses system.nfs4_acl, and
> ext4 which uses system.posix_acl_access.

FYI,
Aufs had met the same problem many years ago, and I introduced some
options called ICEX (Ignore Copyup Error on Xattr).

(from the design doc in aufs)
----------------------------------------
For example, the src branch supports ACL but the dst branch doesn't
because the dst branch may natively un-support it or temporary
un-support it due to "noacl" mount option. Of course, the dst branch fs
may NOT return an error even if the XATTR is not supported. It is
totally up to the branch fs.

Anyway when the aufs internal copy-up gets an error from the dst branch
fs, then aufs tries removing the just copied entry and returns the error
to the userspace. The worst case of this situation will be all copy-up
will fail.

For the copy-up operation, there two basic approaches.
- copy the specified XATTR only (by category above), and return the
  error unconditionally if it happens.
- copy all XATTR, and ignore the error on the specified category only.

In order to support XATTR and to implement the correct behaviour, aufs
chooses the latter approach and introduces some new branch attributes,
"icexsec", "icexsys", "icextr", "icexusr", and "icexoth".
They correspond to the XATTR namespaces (see above). Additionally, to be
convenient, "icex" is also provided which means all "icex*" attributes
are set (here the word "icex" stands for "ignore copy-error on XATTR").

The meaning of these attributes is to ignore the error from setting
XATTR on that branch.
Note that aufs tries copying all XATTR unconditionally, and ignores the
error from the dst branch according to the specified attributes.
----------------------------------------

But recent nfs4 got changed its behaviour around ACL, and it didn't pass
my local tests.  I had posted about that, but got no reply.


J. R. Okajima
NeilBrown May 2, 2019, 11:19 p.m. UTC | #4
On Thu, May 02 2019, Amir Goldstein wrote:

> On Thu, May 2, 2019 at 12:35 AM NeilBrown <neilb@suse.com> wrote:
>>
>>
>> If the upper and lower layers use incompatible ACL formats, it is not
>> possible to copy the ACL xttr from one to the other, so overlayfs
>> cannot work with them.
>> This happens particularly with NFSv4 which uses system.nfs4_acl, and
>> ext4 which uses system.posix_acl_access.
>>
>> If all ACLs actually make to Unix permissions, then there is no need
>> to copy up the ACLs, but overlayfs cannot determine this.
>>
>> So allow the sysadmin it assert that ACLs are not needed with a mount
>> option
>>   honoracl=off
>> This causes the ACLs to not be copied, so filesystems with different
>> ACL formats can be overlaid together.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  Documentation/filesystems/overlayfs.txt | 24 ++++++++++++++++++++++++
>>  fs/overlayfs/copy_up.c                  |  9 +++++++--
>>  fs/overlayfs/dir.c                      |  2 +-
>>  fs/overlayfs/overlayfs.h                |  2 +-
>>  fs/overlayfs/ovl_entry.h                |  1 +
>>  fs/overlayfs/super.c                    | 15 +++++++++++++++
>>  6 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> index eef7d9d259e8..7ad675940c93 100644
>> --- a/Documentation/filesystems/overlayfs.txt
>> +++ b/Documentation/filesystems/overlayfs.txt
>> @@ -245,6 +245,30 @@ filesystem - future operations on the file are barely noticed by the
>>  overlay filesystem (though an operation on the name of the file such as
>>  rename or unlink will of course be noticed and handled).
>>
>> +ACL copy-up
>> +-----------
>> +
>> +When a file that only exists on the lower layer is modified it needs
>> +to be copied up to the upper layer.  This means copying the metadata
>> +and (usually) the data (though see "Metadata only copy up" below).
>> +One part of the metadata can be problematic: the ACLs.
>> +
>> +Now all filesystems support ACLs, and when they do they don't all use
>> +the same format.  A significant conflict appears between POSIX acls
>> +used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There
>> +two formats are, in general, not inter-convertible.
>> +
>> +If a site only uses regular Unix permissions (Read, Write, eXecute by
>> +User, Group and Other), then as these permissions are compatible with
>> +all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
>> +if this is the case itself.
>> +
>> +For this reason, overlayfs supports a mount option "honoracl=off"
>> +which causes ACLs, any "system." extended attribute, on the lower
>> +layer to be ignored and, particularly, not copied to the upper later.
>> +This allows NFSv4 to be overlaid with a local filesystem, but should
>> +only be used if the only access controls used on the filesystem are
>> +Unix permission bits.
>>
>
> I don't know. On the one hand "system." is not only ACLs.

Isn't it?  What else goes in "system." "??

"man xattr" says:

   Extended system attributes
       Extended system attributes are used by the kernel to store system
       objects  such  as  Access Control Lists.  Read and write access
       permissions to system attributes depend on the policy implemented
       for each system attribute implemented by filesystems in the
       kernel.

so it *allows* things other than ACLs, but doesn't confirm that there
are any.

In the kernel source, "XATTR_SYSTEM_PREFIX" is only used with POSIX acls
and "system.sockprotoname" - which is socket specific and no likely to
be found on a filesystem.

"system.
also appears in
   CIFS_XATTR_CIFS_ACL
   SMB3_XATTR_CIFS_ACL
   F2FS_SYSTEM_ADVISE_NAME
   XATTR_NAME_NFSV4_ACL
   SYSTEM_ORANGEFS_KEY

which should all use XATTR_SYSTEM_PREFIX ...

So yes,  I guess they aren't (quite) all ACLs.  Bother.


> On the other hand, "honoracl=off" is not the same as -o noacl,
> but it sure sounds the same.
>
> I'd be a lot more comfortable with "ignore_xattrs=system.nfs4_acl"
> argument takes a comma separated list of xattr prefixes to ignore.

That requires the sysadmin to know a lot more about the internals of the
relevant filesystems.... Maybe that is a good idea, but it feels rather
clunky.

In each of these cases, except maybe POSIX_ACLs, it doesn't make sense
to copy-up the "system." xattr unless it is the exact same filesystem
type.

So if given a "noacl" flag (or similar), ignoring copy-up failure for
all "system." attributes is probably the right thing to do, as ACLs are
the only system. attribute for which it can make any sense at all to
copy them.

Thanks,
NeilBrown


>
> ovl_is_private_xattr() can be generalized to ovl_is_ignored_xattr(),
> going over a blacklist of N>=1 which will also be called from
> ovl_can_list(), because there is no point in listing the ACLs that
> are ignored. right?
>
> Thanks,
> Amir.
diff mbox series

Patch

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index eef7d9d259e8..7ad675940c93 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -245,6 +245,30 @@  filesystem - future operations on the file are barely noticed by the
 overlay filesystem (though an operation on the name of the file such as
 rename or unlink will of course be noticed and handled).
 
+ACL copy-up
+-----------
+
+When a file that only exists on the lower layer is modified it needs
+to be copied up to the upper layer.  This means copying the metadata
+and (usually) the data (though see "Metadata only copy up" below).
+One part of the metadata can be problematic: the ACLs.
+
+Now all filesystems support ACLs, and when they do they don't all use
+the same format.  A significant conflict appears between POSIX acls
+used on many local filesystems, and NFSv4 ACLs used with NFSv4.  There
+two formats are, in general, not inter-convertible.
+
+If a site only uses regular Unix permissions (Read, Write, eXecute by
+User, Group and Other), then as these permissions are compatible with
+all ACLs, there is no need to copy ACLs.  overlayfs cannot determine
+if this is the case itself.
+
+For this reason, overlayfs supports a mount option "honoracl=off"
+which causes ACLs, any "system." extended attribute, on the lower
+layer to be ignored and, particularly, not copied to the upper later.
+This allows NFSv4 to be overlaid with a local filesystem, but should
+only be used if the only access controls used on the filesystem are
+Unix permission bits.
 
 Multiple lower layers
 ---------------------
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 68b3303e4b46..032aa88f21c1 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -39,7 +39,7 @@  static int ovl_ccup_get(char *buf, const struct kernel_param *param)
 module_param_call(check_copy_up, ovl_ccup_set, ovl_ccup_get, NULL, 0644);
 MODULE_PARM_DESC(ovl_check_copy_up, "Obsolete; does nothing");
 
-int ovl_copy_xattr(struct dentry *old, struct dentry *new)
+int ovl_copy_xattr(struct dentry *old, struct dentry *new, struct ovl_fs *ofs)
 {
 	ssize_t list_size, size, value_size = 0;
 	char *buf, *name, *value = NULL;
@@ -77,6 +77,10 @@  int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 		}
 		list_size -= slen;
 
+		if (strncmp(name, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN) == 0 &&
+		    !ofs->config.honoracl)
+			continue;
+
 		if (ovl_is_private_xattr(name))
 			continue;
 retry:
@@ -461,7 +465,8 @@  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
-	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
+	err = ovl_copy_xattr(c->lowerpath.dentry, temp,
+			     c->dentry->d_sb->s_fs_info);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 82c129bfe58d..cc8fb9eeb7df 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -368,7 +368,7 @@  static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (IS_ERR(opaquedir))
 		goto out_unlock;
 
-	err = ovl_copy_xattr(upper, opaquedir);
+	err = ovl_copy_xattr(upper, opaquedir, upper->d_sb->s_fs_info);
 	if (err)
 		goto out_cleanup;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 9c6018287d57..4a104a4732af 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -422,7 +422,7 @@  int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_with_data(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
 int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags);
-int ovl_copy_xattr(struct dentry *old, struct dentry *new);
+int ovl_copy_xattr(struct dentry *old, struct dentry *new, struct ovl_fs *ofs);
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
 struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
 int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..c541e3fed5b9 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -20,6 +20,7 @@  struct ovl_config {
 	bool nfs_export;
 	int xino;
 	bool metacopy;
+	bool honoracl;
 };
 
 struct ovl_sb {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0116735cc321..ceb8fdb7ce14 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -362,6 +362,8 @@  static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.metacopy != ovl_metacopy_def)
 		seq_printf(m, ",metacopy=%s",
 			   ofs->config.metacopy ? "on" : "off");
+	if (!ofs->config.honoracl)
+		seq_puts(m, ",honoracl=off");
 	return 0;
 }
 
@@ -401,6 +403,8 @@  enum {
 	OPT_XINO_AUTO,
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
+	OPT_HONORACL_ON,
+	OPT_HONORACL_OFF,
 	OPT_ERR,
 };
 
@@ -419,6 +423,8 @@  static const match_table_t ovl_tokens = {
 	{OPT_XINO_AUTO,			"xino=auto"},
 	{OPT_METACOPY_ON,		"metacopy=on"},
 	{OPT_METACOPY_OFF,		"metacopy=off"},
+	{OPT_HONORACL_ON,		"honoracl=on"},
+	{OPT_HONORACL_OFF,		"honoracl=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -557,6 +563,14 @@  static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->metacopy = false;
 			break;
 
+		case OPT_HONORACL_ON:
+			config->honoracl = true;
+			break;
+
+		case OPT_HONORACL_OFF:
+			config->honoracl = false;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
@@ -1440,6 +1454,7 @@  static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ofs->config.nfs_export = ovl_nfs_export_def;
 	ofs->config.xino = ovl_xino_def();
 	ofs->config.metacopy = ovl_metacopy_def;
+	ofs->config.honoracl = true;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;