diff mbox

posix acls: Move namespace conversion into filesystem / xattr handlers

Message ID 1464008989-3812-1-git-send-email-agruenba@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Gruenbacher May 23, 2016, 1:09 p.m. UTC
Currently, getxattr() and setxattr() check for the xattr names
"system.posix_acl_{access,default}" and perform in-place UID / GID
namespace mappings in the xattr values. Filesystems then again check for
the same xattr names to handle those attributes, almost always using the
standard posix_acl_{access,default}_xattr_handler handlers.  This is
unnecessary overhead; move the namespace conversion into the xattr
handlers instead.

For filesystems that use the POSIX ACL xattr handlers, no change is
required.  Filesystems that don't use those handlers (cifs and lustre)
need to take care of the namespace mapping themselves now.

The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
lustre, so this patch moves them into lustre.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---

I'm reasonably confident about the core and cifs changes in this patch.
The lustre code is pretty weird though, so could I please get a careful
review on the changes there?

Thanks,
Andreas

 drivers/staging/lustre/lustre/llite/Makefile       |  1 +
 .../staging/lustre/lustre/llite/llite_internal.h   |  3 ++
 drivers/staging/lustre/lustre/llite/posix_acl.c    | 62 ++++++++++++++++++++++
 drivers/staging/lustre/lustre/llite/xattr.c        |  8 ++-
 fs/cifs/cifssmb.c                                  | 41 ++++++++++----
 fs/posix_acl.c                                     | 62 +---------------------
 fs/xattr.c                                         |  7 ---
 include/linux/posix_acl_xattr.h                    | 12 -----
 8 files changed, 107 insertions(+), 89 deletions(-)
 create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c

Comments

James Simmons May 23, 2016, 9:06 p.m. UTC | #1
> Currently, getxattr() and setxattr() check for the xattr names
> "system.posix_acl_{access,default}" and perform in-place UID / GID
> namespace mappings in the xattr values. Filesystems then again check for
> the same xattr names to handle those attributes, almost always using the
> standard posix_acl_{access,default}_xattr_handler handlers.  This is
> unnecessary overhead; move the namespace conversion into the xattr
> handlers instead.
> 
> For filesystems that use the POSIX ACL xattr handlers, no change is
> required.  Filesystems that don't use those handlers (cifs and lustre)
> need to take care of the namespace mapping themselves now.
> 
> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> lustre, so this patch moves them into lustre.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
> 
> I'm reasonably confident about the core and cifs changes in this patch.
> The lustre code is pretty weird though, so could I please get a careful
> review on the changes there?

Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
Besides POSIX ACLs lustre has created a extended ACL as well that is 
grouped in with posix ACL handling. This extended ACL is like POSIX acls 
except it also contains the state (deleted, modified, ...) of the ACL. 
Besides normal local access control list handling Lustre manages remote
access control list handling. You can read about this handling is in 
llite_rmtacl.c. This code was written long before I became involved with
lustre so the exact details are fuzzy to me. The guts of this are handled 
is located at:

drivers/staging/lustre/lustre/obdclass/acl.c

P.S

    A you probably have noticed Lustre has had an uptick in development
which means the code is now changing all the time in staging. How should
we handle the changes you are working in your own trees verses what is
happing in staging. For example I'm looking at moving lustre to the
xattr_handles. Should I push it to you and wait until it works it way
into Greg's tree. What do the merge schedules look like. Lastly the
a_refcount for the POSIX acl changed with your xattr updates which now
causes lustre to crash :-(
 
> Thanks,
> Andreas
> 
>  drivers/staging/lustre/lustre/llite/Makefile       |  1 +
>  .../staging/lustre/lustre/llite/llite_internal.h   |  3 ++
>  drivers/staging/lustre/lustre/llite/posix_acl.c    | 62 ++++++++++++++++++++++
>  drivers/staging/lustre/lustre/llite/xattr.c        |  8 ++-
>  fs/cifs/cifssmb.c                                  | 41 ++++++++++----
>  fs/posix_acl.c                                     | 62 +---------------------
>  fs/xattr.c                                         |  7 ---
>  include/linux/posix_acl_xattr.h                    | 12 -----
>  8 files changed, 107 insertions(+), 89 deletions(-)
>  create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
> 
> diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile
> index 2ce10ff..67125f8 100644
> --- a/drivers/staging/lustre/lustre/llite/Makefile
> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \
>  	    glimpse.o lcommon_cl.o lcommon_misc.o \
>  	    vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
>  	    lproc_llite.o
> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
>  
>  llite_lloop-y := lloop.o
> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> index ce1f949..d454dfb 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
>  __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
>  __u32 cl_fid_build_gen(const struct lu_fid *fid);
>  
> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> +
>  #endif /* LLITE_INTERNAL_H */
> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c
> new file mode 100644
> index 0000000..4fabd0f
> --- /dev/null
> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
> @@ -0,0 +1,62 @@
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/posix_acl_xattr.h>
> +#include <linux/user_namespace.h>
> +
> +/*
> + * Fix up the uids and gids in posix acl extended attributes in place.
> + */
> +static void posix_acl_fix_xattr_userns(
> +	struct user_namespace *to, struct user_namespace *from,
> +	void *value, size_t size)
> +{
> +	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> +	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> +	int count;
> +	kuid_t uid;
> +	kgid_t gid;
> +
> +	if (!value)
> +		return;
> +	if (size < sizeof(posix_acl_xattr_header))
> +		return;
> +	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> +		return;
> +
> +	count = posix_acl_xattr_count(size);
> +	if (count < 0)
> +		return;
> +	if (count == 0)
> +		return;
> +
> +	for (end = entry + count; entry != end; entry++) {
> +		switch(le16_to_cpu(entry->e_tag)) {
> +		case ACL_USER:
> +			uid = make_kuid(from, le32_to_cpu(entry->e_id));
> +			entry->e_id = cpu_to_le32(from_kuid(to, uid));
> +			break;
> +		case ACL_GROUP:
> +			gid = make_kgid(from, le32_to_cpu(entry->e_id));
> +			entry->e_id = cpu_to_le32(from_kgid(to, gid));
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +void posix_acl_fix_xattr_from_user(void *value, size_t size)
> +{
> +	struct user_namespace *user_ns = current_user_ns();
> +	if (user_ns == &init_user_ns)
> +		return;
> +	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> +}
> +
> +void posix_acl_fix_xattr_to_user(void *value, size_t size)
> +{
> +	struct user_namespace *user_ns = current_user_ns();
> +	if (user_ns == &init_user_ns)
> +		return;
> +	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> +}
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index ed4de04..c721b44 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -144,6 +144,9 @@ int ll_setxattr_common(struct inode *inode, const char *name,
>  		return -EOPNOTSUPP;
>  
>  #ifdef CONFIG_FS_POSIX_ACL
> +	if (xattr_type == XATTR_ACL_ACCESS_T ||
> +	    xattr_type == XATTR_ACL_DEFAULT_T)
> +		posix_acl_fix_xattr_from_user((void *)value, size);
>  	if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&
>  	    (xattr_type == XATTR_ACL_ACCESS_T ||
>  	    xattr_type == XATTR_ACL_DEFAULT_T)) {
> @@ -348,7 +351,7 @@ int ll_getxattr_common(struct inode *inode, const char *name,
>  		if (!acl)
>  			return -ENODATA;
>  
> -		rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> +		rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);
>  		posix_acl_release(acl);
>  		return rc;
>  	}
> @@ -436,6 +439,9 @@ getxattr_nocache:
>  			goto out;
>  		}
>  	}
> +	if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||
> +			xattr_type == XATTR_ACL_DEFAULT_T))
> +		posix_acl_fix_xattr_to_user(buffer, rc);
>  #endif
>  
>  out_xattr:
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index d47197e..9dc001f 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -3337,10 +3337,25 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
>  static void cifs_convert_ace(posix_acl_xattr_entry *ace,
>  			     struct cifs_posix_ace *cifs_ace)
>  {
> +	u32 cifs_id, id = -1;
> +
>  	/* u8 cifs fields do not need le conversion */
>  	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
>  	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
> -	ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
> +	switch(cifs_ace->cifs_e_tag) {
> +	case ACL_USER:
> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> +		id = from_kuid(current_user_ns(),
> +			       make_kuid(&init_user_ns, cifs_id));
> +		break;
> +
> +	case ACL_GROUP:
> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> +		id = from_kgid(current_user_ns(),
> +			       make_kgid(&init_user_ns, cifs_id));
> +		break;
> +	}
> +	ace->e_id = cpu_to_le32(id);
>  /*
>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",
>  		 ace->e_perm, ace->e_tag, ace->e_id);
> @@ -3408,21 +3423,29 @@ static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
>  static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
>  				     const posix_acl_xattr_entry *local_ace)
>  {
> -	__u16 rc = 0; /* 0 = ACL converted ok */
> +	u32 cifs_id = -1, id;
>  
>  	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
>  	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
> -	/* BB is there a better way to handle the large uid? */
> -	if (local_ace->e_id == cpu_to_le32(-1)) {
> -	/* Probably no need to le convert -1 on any arch but can not hurt */
> -		cifs_ace->cifs_uid = cpu_to_le64(-1);
> -	} else
> -		cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
> +	switch(cifs_ace->cifs_e_tag) {
> +	case ACL_USER:
> +		id = le32_to_cpu(local_ace->e_id);
> +		cifs_id = from_kuid(&init_user_ns,
> +				    make_kuid(current_user_ns(), id));
> +		break;
> +
> +	case ACL_GROUP:
> +		id = le32_to_cpu(local_ace->e_id);
> +		cifs_id = from_kgid(&init_user_ns,
> +				    make_kgid(current_user_ns(), id));
> +		break;
> +	}
> +	cifs_ace->cifs_uid = cpu_to_le64(cifs_id);
>  /*
>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",
>  		 ace->e_perm, ace->e_tag, ace->e_id);
>  */
> -	return rc;
> +	return 0;
>  }
>  
>  /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 2c60f17..fca281c 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -627,64 +627,6 @@ no_mem:
>  EXPORT_SYMBOL_GPL(posix_acl_create);
>  
>  /*
> - * Fix up the uids and gids in posix acl extended attributes in place.
> - */
> -static void posix_acl_fix_xattr_userns(
> -	struct user_namespace *to, struct user_namespace *from,
> -	void *value, size_t size)
> -{
> -	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> -	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> -	int count;
> -	kuid_t uid;
> -	kgid_t gid;
> -
> -	if (!value)
> -		return;
> -	if (size < sizeof(posix_acl_xattr_header))
> -		return;
> -	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> -		return;
> -
> -	count = posix_acl_xattr_count(size);
> -	if (count < 0)
> -		return;
> -	if (count == 0)
> -		return;
> -
> -	for (end = entry + count; entry != end; entry++) {
> -		switch(le16_to_cpu(entry->e_tag)) {
> -		case ACL_USER:
> -			uid = make_kuid(from, le32_to_cpu(entry->e_id));
> -			entry->e_id = cpu_to_le32(from_kuid(to, uid));
> -			break;
> -		case ACL_GROUP:
> -			gid = make_kgid(from, le32_to_cpu(entry->e_id));
> -			entry->e_id = cpu_to_le32(from_kgid(to, gid));
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> -}
> -
> -void posix_acl_fix_xattr_from_user(void *value, size_t size)
> -{
> -	struct user_namespace *user_ns = current_user_ns();
> -	if (user_ns == &init_user_ns)
> -		return;
> -	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> -}
> -
> -void posix_acl_fix_xattr_to_user(void *value, size_t size)
> -{
> -	struct user_namespace *user_ns = current_user_ns();
> -	if (user_ns == &init_user_ns)
> -		return;
> -	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> -}
> -
> -/*
>   * Convert from extended attribute to in-memory representation.
>   */
>  struct posix_acl *
> @@ -814,7 +756,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,
>  	if (acl == NULL)
>  		return -ENODATA;
>  
> -	error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +	error = posix_acl_to_xattr(current_user_ns(), acl, value, size);
>  	posix_acl_release(acl);
>  
>  	return error;
> @@ -840,7 +782,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
>  		return -EPERM;
>  
>  	if (value) {
> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +		acl = posix_acl_from_xattr(current_user_ns(), value, size);
>  		if (IS_ERR(acl))
>  			return PTR_ERR(acl);
>  
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b11945e..b648b05 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -20,7 +20,6 @@
>  #include <linux/fsnotify.h>
>  #include <linux/audit.h>
>  #include <linux/vmalloc.h>
> -#include <linux/posix_acl_xattr.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -329,9 +328,6 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
>  			error = -EFAULT;
>  			goto out;
>  		}
> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> -			posix_acl_fix_xattr_from_user(kvalue, size);
>  	}
>  
>  	error = vfs_setxattr(d, kname, kvalue, size, flags);
> @@ -426,9 +422,6 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>  
>  	error = vfs_getxattr(d, kname, kvalue, size);
>  	if (error > 0) {
> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> -			posix_acl_fix_xattr_to_user(kvalue, size);
>  		if (size && copy_to_user(value, kvalue, error))
>  			error = -EFAULT;
>  	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> index e5e8ec4..9789aba 100644
> --- a/include/linux/posix_acl_xattr.h
> +++ b/include/linux/posix_acl_xattr.h
> @@ -48,18 +48,6 @@ posix_acl_xattr_count(size_t size)
>  	return size / sizeof(posix_acl_xattr_entry);
>  }
>  
> -#ifdef CONFIG_FS_POSIX_ACL
> -void posix_acl_fix_xattr_from_user(void *value, size_t size);
> -void posix_acl_fix_xattr_to_user(void *value, size_t size);
> -#else
> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
> -{
> -}
> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
> -{
> -}
> -#endif
> -
>  struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns, 
>  				       const void *value, size_t size);
>  int posix_acl_to_xattr(struct user_namespace *user_ns,
> -- 
> 2.5.5
> 
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Grünbacher May 23, 2016, 10:30 p.m. UTC | #2
2016-05-23 23:06 GMT+02:00 James Simmons <jsimmons@infradead.org>:
> Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
> Besides POSIX ACLs lustre has created a extended ACL as well that is
> grouped in with posix ACL handling. This extended ACL is like POSIX acls
> except it also contains the state (deleted, modified, ...) of the ACL.
> Besides normal local access control list handling Lustre manages remote
> access control list handling. You can read about this handling is in
> llite_rmtacl.c. This code was written long before I became involved with
> lustre so the exact details are fuzzy to me. The guts of this are handled
> is located at:
>
> drivers/staging/lustre/lustre/obdclass/acl.c

I'm not sure we're talking about the same thing here. As far as I can
see, the only code path from getxattr(2) and settxattr(2) into lustre
is through ll_getxattr and ll_setxattr. So far, the vfs performed the
uid/gid mappings before calling into the filesystem. With this patch,
the filesystem performs this mapping instead. In the lustre case,
pretty much directly after entering ll_getxattr and immediately before
leaving ll_setxattr. The rest of the code has and still does operate
in the "init uid/gid namespace". Did I miss anything?

>     A you probably have noticed Lustre has had an uptick in development
> which means the code is now changing all the time in staging. How should
> we handle the changes you are working in your own trees verses what is
> happing in staging. For example I'm looking at moving lustre to the
> xattr_handles.

Okay, great.

> Should I push it to you and wait until it works it way
> into Greg's tree.

Changing lustre to use xattr handlers does not dependent on any of my
pending xattr changes, only the other way around. Please copy me for
review and merge through Greg's tree as usual. Once I have your
changes on a public branch, I can merge that into my xattr inode
operation removal branch.

> What do the merge schedules look like.

I hope the {get,set,remove}xattr inode operations will go away in the
next merge window, so it would be good to have the lustre xattr
handler convestion on a public branch relatively soon if possible.

> Lastly the a_refcount for the POSIX acl changed with your xattr updates which now
> causes lustre to crash :-(

You mean commit b8a7a3a6 "posix_acl: Inode acl caching fixes"? It
seems a "forget_cached_acl(inode, type);" before returning the acl in
ll_get_acl is missing -- but lustre still shouldn't crash because of
that.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve French May 24, 2016, 1:47 a.m. UTC | #3
Reviewed by: Steve French <steve.french@primarydata.com>
(for the non-lustre pieces of the patch)

On Mon, May 23, 2016 at 8:09 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> Currently, getxattr() and setxattr() check for the xattr names
> "system.posix_acl_{access,default}" and perform in-place UID / GID
> namespace mappings in the xattr values. Filesystems then again check for
> the same xattr names to handle those attributes, almost always using the
> standard posix_acl_{access,default}_xattr_handler handlers.  This is
> unnecessary overhead; move the namespace conversion into the xattr
> handlers instead.
>
> For filesystems that use the POSIX ACL xattr handlers, no change is
> required.  Filesystems that don't use those handlers (cifs and lustre)
> need to take care of the namespace mapping themselves now.
>
> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> lustre, so this patch moves them into lustre.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>
> I'm reasonably confident about the core and cifs changes in this patch.
> The lustre code is pretty weird though, so could I please get a careful
> review on the changes there?
>
> Thanks,
> Andreas
>
>  drivers/staging/lustre/lustre/llite/Makefile       |  1 +
>  .../staging/lustre/lustre/llite/llite_internal.h   |  3 ++
>  drivers/staging/lustre/lustre/llite/posix_acl.c    | 62 ++++++++++++++++++++++
>  drivers/staging/lustre/lustre/llite/xattr.c        |  8 ++-
>  fs/cifs/cifssmb.c                                  | 41 ++++++++++----
>  fs/posix_acl.c                                     | 62 +---------------------
>  fs/xattr.c                                         |  7 ---
>  include/linux/posix_acl_xattr.h                    | 12 -----
>  8 files changed, 107 insertions(+), 89 deletions(-)
>  create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
>
> diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile
> index 2ce10ff..67125f8 100644
> --- a/drivers/staging/lustre/lustre/llite/Makefile
> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \
>             glimpse.o lcommon_cl.o lcommon_misc.o \
>             vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
>             lproc_llite.o
> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
>
>  llite_lloop-y := lloop.o
> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> index ce1f949..d454dfb 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
>  __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
>  __u32 cl_fid_build_gen(const struct lu_fid *fid);
>
> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> +
>  #endif /* LLITE_INTERNAL_H */
> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c
> new file mode 100644
> index 0000000..4fabd0f
> --- /dev/null
> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
> @@ -0,0 +1,62 @@
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/posix_acl_xattr.h>
> +#include <linux/user_namespace.h>
> +
> +/*
> + * Fix up the uids and gids in posix acl extended attributes in place.
> + */
> +static void posix_acl_fix_xattr_userns(
> +       struct user_namespace *to, struct user_namespace *from,
> +       void *value, size_t size)
> +{
> +       posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> +       posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> +       int count;
> +       kuid_t uid;
> +       kgid_t gid;
> +
> +       if (!value)
> +               return;
> +       if (size < sizeof(posix_acl_xattr_header))
> +               return;
> +       if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> +               return;
> +
> +       count = posix_acl_xattr_count(size);
> +       if (count < 0)
> +               return;
> +       if (count == 0)
> +               return;
> +
> +       for (end = entry + count; entry != end; entry++) {
> +               switch(le16_to_cpu(entry->e_tag)) {
> +               case ACL_USER:
> +                       uid = make_kuid(from, le32_to_cpu(entry->e_id));
> +                       entry->e_id = cpu_to_le32(from_kuid(to, uid));
> +                       break;
> +               case ACL_GROUP:
> +                       gid = make_kgid(from, le32_to_cpu(entry->e_id));
> +                       entry->e_id = cpu_to_le32(from_kgid(to, gid));
> +                       break;
> +               default:
> +                       break;
> +               }
> +       }
> +}
> +
> +void posix_acl_fix_xattr_from_user(void *value, size_t size)
> +{
> +       struct user_namespace *user_ns = current_user_ns();
> +       if (user_ns == &init_user_ns)
> +               return;
> +       posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> +}
> +
> +void posix_acl_fix_xattr_to_user(void *value, size_t size)
> +{
> +       struct user_namespace *user_ns = current_user_ns();
> +       if (user_ns == &init_user_ns)
> +               return;
> +       posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> +}
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index ed4de04..c721b44 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -144,6 +144,9 @@ int ll_setxattr_common(struct inode *inode, const char *name,
>                 return -EOPNOTSUPP;
>
>  #ifdef CONFIG_FS_POSIX_ACL
> +       if (xattr_type == XATTR_ACL_ACCESS_T ||
> +           xattr_type == XATTR_ACL_DEFAULT_T)
> +               posix_acl_fix_xattr_from_user((void *)value, size);
>         if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&
>             (xattr_type == XATTR_ACL_ACCESS_T ||
>             xattr_type == XATTR_ACL_DEFAULT_T)) {
> @@ -348,7 +351,7 @@ int ll_getxattr_common(struct inode *inode, const char *name,
>                 if (!acl)
>                         return -ENODATA;
>
> -               rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> +               rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);
>                 posix_acl_release(acl);
>                 return rc;
>         }
> @@ -436,6 +439,9 @@ getxattr_nocache:
>                         goto out;
>                 }
>         }
> +       if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||
> +                       xattr_type == XATTR_ACL_DEFAULT_T))
> +               posix_acl_fix_xattr_to_user(buffer, rc);
>  #endif
>
>  out_xattr:
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index d47197e..9dc001f 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -3337,10 +3337,25 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
>  static void cifs_convert_ace(posix_acl_xattr_entry *ace,
>                              struct cifs_posix_ace *cifs_ace)
>  {
> +       u32 cifs_id, id = -1;
> +
>         /* u8 cifs fields do not need le conversion */
>         ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
>         ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
> -       ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
> +       switch(cifs_ace->cifs_e_tag) {
> +       case ACL_USER:
> +               cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> +               id = from_kuid(current_user_ns(),
> +                              make_kuid(&init_user_ns, cifs_id));
> +               break;
> +
> +       case ACL_GROUP:
> +               cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> +               id = from_kgid(current_user_ns(),
> +                              make_kgid(&init_user_ns, cifs_id));
> +               break;
> +       }
> +       ace->e_id = cpu_to_le32(id);
>  /*
>         cifs_dbg(FYI, "perm %d tag %d id %d\n",
>                  ace->e_perm, ace->e_tag, ace->e_id);
> @@ -3408,21 +3423,29 @@ static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
>  static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
>                                      const posix_acl_xattr_entry *local_ace)
>  {
> -       __u16 rc = 0; /* 0 = ACL converted ok */
> +       u32 cifs_id = -1, id;
>
>         cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
>         cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
> -       /* BB is there a better way to handle the large uid? */
> -       if (local_ace->e_id == cpu_to_le32(-1)) {
> -       /* Probably no need to le convert -1 on any arch but can not hurt */
> -               cifs_ace->cifs_uid = cpu_to_le64(-1);
> -       } else
> -               cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
> +       switch(cifs_ace->cifs_e_tag) {
> +       case ACL_USER:
> +               id = le32_to_cpu(local_ace->e_id);
> +               cifs_id = from_kuid(&init_user_ns,
> +                                   make_kuid(current_user_ns(), id));
> +               break;
> +
> +       case ACL_GROUP:
> +               id = le32_to_cpu(local_ace->e_id);
> +               cifs_id = from_kgid(&init_user_ns,
> +                                   make_kgid(current_user_ns(), id));
> +               break;
> +       }
> +       cifs_ace->cifs_uid = cpu_to_le64(cifs_id);
>  /*
>         cifs_dbg(FYI, "perm %d tag %d id %d\n",
>                  ace->e_perm, ace->e_tag, ace->e_id);
>  */
> -       return rc;
> +       return 0;
>  }
>
>  /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 2c60f17..fca281c 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -627,64 +627,6 @@ no_mem:
>  EXPORT_SYMBOL_GPL(posix_acl_create);
>
>  /*
> - * Fix up the uids and gids in posix acl extended attributes in place.
> - */
> -static void posix_acl_fix_xattr_userns(
> -       struct user_namespace *to, struct user_namespace *from,
> -       void *value, size_t size)
> -{
> -       posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> -       posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> -       int count;
> -       kuid_t uid;
> -       kgid_t gid;
> -
> -       if (!value)
> -               return;
> -       if (size < sizeof(posix_acl_xattr_header))
> -               return;
> -       if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> -               return;
> -
> -       count = posix_acl_xattr_count(size);
> -       if (count < 0)
> -               return;
> -       if (count == 0)
> -               return;
> -
> -       for (end = entry + count; entry != end; entry++) {
> -               switch(le16_to_cpu(entry->e_tag)) {
> -               case ACL_USER:
> -                       uid = make_kuid(from, le32_to_cpu(entry->e_id));
> -                       entry->e_id = cpu_to_le32(from_kuid(to, uid));
> -                       break;
> -               case ACL_GROUP:
> -                       gid = make_kgid(from, le32_to_cpu(entry->e_id));
> -                       entry->e_id = cpu_to_le32(from_kgid(to, gid));
> -                       break;
> -               default:
> -                       break;
> -               }
> -       }
> -}
> -
> -void posix_acl_fix_xattr_from_user(void *value, size_t size)
> -{
> -       struct user_namespace *user_ns = current_user_ns();
> -       if (user_ns == &init_user_ns)
> -               return;
> -       posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> -}
> -
> -void posix_acl_fix_xattr_to_user(void *value, size_t size)
> -{
> -       struct user_namespace *user_ns = current_user_ns();
> -       if (user_ns == &init_user_ns)
> -               return;
> -       posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> -}
> -
> -/*
>   * Convert from extended attribute to in-memory representation.
>   */
>  struct posix_acl *
> @@ -814,7 +756,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,
>         if (acl == NULL)
>                 return -ENODATA;
>
> -       error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +       error = posix_acl_to_xattr(current_user_ns(), acl, value, size);
>         posix_acl_release(acl);
>
>         return error;
> @@ -840,7 +782,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
>                 return -EPERM;
>
>         if (value) {
> -               acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +               acl = posix_acl_from_xattr(current_user_ns(), value, size);
>                 if (IS_ERR(acl))
>                         return PTR_ERR(acl);
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b11945e..b648b05 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -20,7 +20,6 @@
>  #include <linux/fsnotify.h>
>  #include <linux/audit.h>
>  #include <linux/vmalloc.h>
> -#include <linux/posix_acl_xattr.h>
>
>  #include <asm/uaccess.h>
>
> @@ -329,9 +328,6 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
>                         error = -EFAULT;
>                         goto out;
>                 }
> -               if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> -                   (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> -                       posix_acl_fix_xattr_from_user(kvalue, size);
>         }
>
>         error = vfs_setxattr(d, kname, kvalue, size, flags);
> @@ -426,9 +422,6 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>
>         error = vfs_getxattr(d, kname, kvalue, size);
>         if (error > 0) {
> -               if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> -                   (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> -                       posix_acl_fix_xattr_to_user(kvalue, size);
>                 if (size && copy_to_user(value, kvalue, error))
>                         error = -EFAULT;
>         } else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> index e5e8ec4..9789aba 100644
> --- a/include/linux/posix_acl_xattr.h
> +++ b/include/linux/posix_acl_xattr.h
> @@ -48,18 +48,6 @@ posix_acl_xattr_count(size_t size)
>         return size / sizeof(posix_acl_xattr_entry);
>  }
>
> -#ifdef CONFIG_FS_POSIX_ACL
> -void posix_acl_fix_xattr_from_user(void *value, size_t size);
> -void posix_acl_fix_xattr_to_user(void *value, size_t size);
> -#else
> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
> -{
> -}
> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
> -{
> -}
> -#endif
> -
>  struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns,
>                                        const void *value, size_t size);
>  int posix_acl_to_xattr(struct user_namespace *user_ns,
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 24, 2016, 8:47 a.m. UTC | #4
On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
> I'm reasonably confident about the core and cifs changes in this patch.
> The lustre code is pretty weird though, so could I please get a careful
> review on the changes there?

Lustre is in the staging tree because it's an unrescuable piece of junk.
Please just ignore it and move the proper parts of the kernel forward.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Djalal Harouni May 24, 2016, 3:41 p.m. UTC | #5
Hi,

On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
> Currently, getxattr() and setxattr() check for the xattr names
> "system.posix_acl_{access,default}" and perform in-place UID / GID
> namespace mappings in the xattr values. Filesystems then again check for
> the same xattr names to handle those attributes, almost always using the
> standard posix_acl_{access,default}_xattr_handler handlers.  This is
> unnecessary overhead; move the namespace conversion into the xattr
> handlers instead.
Please, are you sure that the changes in posix_acl_xattr_get() and
posix_acl_xattr_set() are safe ? you are reading into current user
namespace, from a first view this is not safe unless I'm missing
something... they should map into init_user_ns...

Please Cc the user namespace maintainers before. Thank you!


> For filesystems that use the POSIX ACL xattr handlers, no change is
> required.  Filesystems that don't use those handlers (cifs and lustre)
> need to take care of the namespace mapping themselves now.
> 
> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> lustre, so this patch moves them into lustre.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
> 
> I'm reasonably confident about the core and cifs changes in this patch.
> The lustre code is pretty weird though, so could I please get a careful
> review on the changes there?
> 
> Thanks,
> Andreas
> 
>  drivers/staging/lustre/lustre/llite/Makefile       |  1 +
>  .../staging/lustre/lustre/llite/llite_internal.h   |  3 ++
>  drivers/staging/lustre/lustre/llite/posix_acl.c    | 62 ++++++++++++++++++++++
>  drivers/staging/lustre/lustre/llite/xattr.c        |  8 ++-
>  fs/cifs/cifssmb.c                                  | 41 ++++++++++----
>  fs/posix_acl.c                                     | 62 +---------------------
>  fs/xattr.c                                         |  7 ---
>  include/linux/posix_acl_xattr.h                    | 12 -----
>  8 files changed, 107 insertions(+), 89 deletions(-)
>  create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
> 
> diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile
> index 2ce10ff..67125f8 100644
> --- a/drivers/staging/lustre/lustre/llite/Makefile
> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \
>  	    glimpse.o lcommon_cl.o lcommon_misc.o \
>  	    vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
>  	    lproc_llite.o
> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
>  
>  llite_lloop-y := lloop.o
> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> index ce1f949..d454dfb 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
>  __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
>  __u32 cl_fid_build_gen(const struct lu_fid *fid);
>  
> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> +
>  #endif /* LLITE_INTERNAL_H */
> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c
> new file mode 100644
> index 0000000..4fabd0f
> --- /dev/null
> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
> @@ -0,0 +1,62 @@
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/posix_acl_xattr.h>
> +#include <linux/user_namespace.h>
> +
> +/*
> + * Fix up the uids and gids in posix acl extended attributes in place.
> + */
> +static void posix_acl_fix_xattr_userns(
> +	struct user_namespace *to, struct user_namespace *from,
> +	void *value, size_t size)
> +{
> +	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> +	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> +	int count;
> +	kuid_t uid;
> +	kgid_t gid;
> +
> +	if (!value)
> +		return;
> +	if (size < sizeof(posix_acl_xattr_header))
> +		return;
> +	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> +		return;
> +
> +	count = posix_acl_xattr_count(size);
> +	if (count < 0)
> +		return;
> +	if (count == 0)
> +		return;
> +
> +	for (end = entry + count; entry != end; entry++) {
> +		switch(le16_to_cpu(entry->e_tag)) {
> +		case ACL_USER:
> +			uid = make_kuid(from, le32_to_cpu(entry->e_id));
> +			entry->e_id = cpu_to_le32(from_kuid(to, uid));
> +			break;
> +		case ACL_GROUP:
> +			gid = make_kgid(from, le32_to_cpu(entry->e_id));
> +			entry->e_id = cpu_to_le32(from_kgid(to, gid));
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +void posix_acl_fix_xattr_from_user(void *value, size_t size)
> +{
> +	struct user_namespace *user_ns = current_user_ns();
> +	if (user_ns == &init_user_ns)
> +		return;
> +	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> +}
> +
> +void posix_acl_fix_xattr_to_user(void *value, size_t size)
> +{
> +	struct user_namespace *user_ns = current_user_ns();
> +	if (user_ns == &init_user_ns)
> +		return;
> +	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> +}
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index ed4de04..c721b44 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -144,6 +144,9 @@ int ll_setxattr_common(struct inode *inode, const char *name,
>  		return -EOPNOTSUPP;
>  
>  #ifdef CONFIG_FS_POSIX_ACL
> +	if (xattr_type == XATTR_ACL_ACCESS_T ||
> +	    xattr_type == XATTR_ACL_DEFAULT_T)
> +		posix_acl_fix_xattr_from_user((void *)value, size);
>  	if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&
>  	    (xattr_type == XATTR_ACL_ACCESS_T ||
>  	    xattr_type == XATTR_ACL_DEFAULT_T)) {
> @@ -348,7 +351,7 @@ int ll_getxattr_common(struct inode *inode, const char *name,
>  		if (!acl)
>  			return -ENODATA;
>  
> -		rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> +		rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);
>  		posix_acl_release(acl);
>  		return rc;
>  	}
> @@ -436,6 +439,9 @@ getxattr_nocache:
>  			goto out;
>  		}
>  	}
> +	if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||
> +			xattr_type == XATTR_ACL_DEFAULT_T))
> +		posix_acl_fix_xattr_to_user(buffer, rc);
>  #endif
>  
>  out_xattr:
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index d47197e..9dc001f 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -3337,10 +3337,25 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
>  static void cifs_convert_ace(posix_acl_xattr_entry *ace,
>  			     struct cifs_posix_ace *cifs_ace)
>  {
> +	u32 cifs_id, id = -1;
> +
>  	/* u8 cifs fields do not need le conversion */
>  	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
>  	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
> -	ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
> +	switch(cifs_ace->cifs_e_tag) {
> +	case ACL_USER:
> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> +		id = from_kuid(current_user_ns(),
> +			       make_kuid(&init_user_ns, cifs_id));
> +		break;
> +
> +	case ACL_GROUP:
> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> +		id = from_kgid(current_user_ns(),
> +			       make_kgid(&init_user_ns, cifs_id));
> +		break;
> +	}
> +	ace->e_id = cpu_to_le32(id);
>  /*
>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",
>  		 ace->e_perm, ace->e_tag, ace->e_id);
> @@ -3408,21 +3423,29 @@ static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
>  static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
>  				     const posix_acl_xattr_entry *local_ace)
>  {
> -	__u16 rc = 0; /* 0 = ACL converted ok */
> +	u32 cifs_id = -1, id;
>  
>  	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
>  	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
> -	/* BB is there a better way to handle the large uid? */
> -	if (local_ace->e_id == cpu_to_le32(-1)) {
> -	/* Probably no need to le convert -1 on any arch but can not hurt */
> -		cifs_ace->cifs_uid = cpu_to_le64(-1);
> -	} else
> -		cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
> +	switch(cifs_ace->cifs_e_tag) {
> +	case ACL_USER:
> +		id = le32_to_cpu(local_ace->e_id);
> +		cifs_id = from_kuid(&init_user_ns,
> +				    make_kuid(current_user_ns(), id));
> +		break;
> +
> +	case ACL_GROUP:
> +		id = le32_to_cpu(local_ace->e_id);
> +		cifs_id = from_kgid(&init_user_ns,
> +				    make_kgid(current_user_ns(), id));
> +		break;
> +	}
> +	cifs_ace->cifs_uid = cpu_to_le64(cifs_id);
>  /*
>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",
>  		 ace->e_perm, ace->e_tag, ace->e_id);
>  */
> -	return rc;
> +	return 0;
>  }
>  
>  /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 2c60f17..fca281c 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -627,64 +627,6 @@ no_mem:
>  EXPORT_SYMBOL_GPL(posix_acl_create);
>  
>  /*
> - * Fix up the uids and gids in posix acl extended attributes in place.
> - */
> -static void posix_acl_fix_xattr_userns(
> -	struct user_namespace *to, struct user_namespace *from,
> -	void *value, size_t size)
> -{
> -	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> -	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> -	int count;
> -	kuid_t uid;
> -	kgid_t gid;
> -
> -	if (!value)
> -		return;
> -	if (size < sizeof(posix_acl_xattr_header))
> -		return;
> -	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> -		return;
> -
> -	count = posix_acl_xattr_count(size);
> -	if (count < 0)
> -		return;
> -	if (count == 0)
> -		return;
> -
> -	for (end = entry + count; entry != end; entry++) {
> -		switch(le16_to_cpu(entry->e_tag)) {
> -		case ACL_USER:
> -			uid = make_kuid(from, le32_to_cpu(entry->e_id));
> -			entry->e_id = cpu_to_le32(from_kuid(to, uid));
> -			break;
> -		case ACL_GROUP:
> -			gid = make_kgid(from, le32_to_cpu(entry->e_id));
> -			entry->e_id = cpu_to_le32(from_kgid(to, gid));
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> -}
> -
> -void posix_acl_fix_xattr_from_user(void *value, size_t size)
> -{
> -	struct user_namespace *user_ns = current_user_ns();
> -	if (user_ns == &init_user_ns)
> -		return;
> -	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> -}
> -
> -void posix_acl_fix_xattr_to_user(void *value, size_t size)
> -{
> -	struct user_namespace *user_ns = current_user_ns();
> -	if (user_ns == &init_user_ns)
> -		return;
> -	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> -}
> -
> -/*
>   * Convert from extended attribute to in-memory representation.
>   */
>  struct posix_acl *
> @@ -814,7 +756,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,
>  	if (acl == NULL)
>  		return -ENODATA;
>  
> -	error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +	error = posix_acl_to_xattr(current_user_ns(), acl, value, size);
>  	posix_acl_release(acl);
>  
>  	return error;
> @@ -840,7 +782,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
>  		return -EPERM;
>  
>  	if (value) {
> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +		acl = posix_acl_from_xattr(current_user_ns(), value, size);
>  		if (IS_ERR(acl))
>  			return PTR_ERR(acl);
>  
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b11945e..b648b05 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -20,7 +20,6 @@
>  #include <linux/fsnotify.h>
>  #include <linux/audit.h>
>  #include <linux/vmalloc.h>
> -#include <linux/posix_acl_xattr.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -329,9 +328,6 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
>  			error = -EFAULT;
>  			goto out;
>  		}
> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> -			posix_acl_fix_xattr_from_user(kvalue, size);
>  	}
>  
>  	error = vfs_setxattr(d, kname, kvalue, size, flags);
> @@ -426,9 +422,6 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>  
>  	error = vfs_getxattr(d, kname, kvalue, size);
>  	if (error > 0) {
> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> -			posix_acl_fix_xattr_to_user(kvalue, size);
>  		if (size && copy_to_user(value, kvalue, error))
>  			error = -EFAULT;
>  	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> index e5e8ec4..9789aba 100644
> --- a/include/linux/posix_acl_xattr.h
> +++ b/include/linux/posix_acl_xattr.h
> @@ -48,18 +48,6 @@ posix_acl_xattr_count(size_t size)
>  	return size / sizeof(posix_acl_xattr_entry);
>  }
>  
> -#ifdef CONFIG_FS_POSIX_ACL
> -void posix_acl_fix_xattr_from_user(void *value, size_t size);
> -void posix_acl_fix_xattr_to_user(void *value, size_t size);
> -#else
> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
> -{
> -}
> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
> -{
> -}
> -#endif
> -
>  struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns, 
>  				       const void *value, size_t size);
>  int posix_acl_to_xattr(struct user_namespace *user_ns,
> -- 
> 2.5.5
> 
> --
> 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
Andreas Gruenbacher May 24, 2016, 4:31 p.m. UTC | #6
On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
>> Currently, getxattr() and setxattr() check for the xattr names
>> "system.posix_acl_{access,default}" and perform in-place UID / GID
>> namespace mappings in the xattr values. Filesystems then again check for
>> the same xattr names to handle those attributes, almost always using the
>> standard posix_acl_{access,default}_xattr_handler handlers.  This is
>> unnecessary overhead; move the namespace conversion into the xattr
>> handlers instead.
>
> Please, are you sure that the changes in posix_acl_xattr_get() and
> posix_acl_xattr_set() are safe ? you are reading into current user
> namespace, from a first view this is not safe unless I'm missing
> something... they should map into init_user_ns...

Yes, moving the namespace conversion from the VFS into those functions
so that we don't have to check for those attributes and parse them
twice is exactly the point of this patch.

> Please Cc the user namespace maintainers before. Thank you!

Eric, Andy, anyone else?

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dilger, Andreas May 24, 2016, 6:28 p.m. UTC | #7
On 2016/05/23, 15:06, "James Simmons" <jsimmons@infradead.org> wrote:
>

>> Currently, getxattr() and setxattr() check for the xattr names

>> "system.posix_acl_{access,default}" and perform in-place UID / GID

>> namespace mappings in the xattr values. Filesystems then again check for

>> the same xattr names to handle those attributes, almost always using the

>> standard posix_acl_{access,default}_xattr_handler handlers.  This is

>> unnecessary overhead; move the namespace conversion into the xattr

>> handlers instead.

>> 

>> For filesystems that use the POSIX ACL xattr handlers, no change is

>> required.  Filesystems that don't use those handlers (cifs and lustre)

>> need to take care of the namespace mapping themselves now.

>> 

>> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is

>> lustre, so this patch moves them into lustre.

>> 

>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

>> ---

>> 

>> I'm reasonably confident about the core and cifs changes in this patch.

>> The lustre code is pretty weird though, so could I please get a careful

>> review on the changes there?

>

>Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.

>Besides POSIX ACLs lustre has created a extended ACL as well that is 

>grouped in with posix ACL handling. This extended ACL is like POSIX acls 

>except it also contains the state (deleted, modified, ...) of the ACL. 

>Besides normal local access control list handling Lustre manages remote

>access control list handling. You can read about this handling is in 

>llite_rmtacl.c. This code was written long before I became involved with

>lustre so the exact details are fuzzy to me.


James,
the remote ACL code never found any usage in the field and can be
deleted.  Please see http://review.whamcloud.com/19789 for details.

Cheers, Andreas

> The guts of this are handled is located at:

>

>drivers/staging/lustre/lustre/obdclass/acl.c

>

>P.S

>

>    A you probably have noticed Lustre has had an uptick in development

>which means the code is now changing all the time in staging. How should

>we handle the changes you are working in your own trees verses what is

>happing in staging. For example I'm looking at moving lustre to the

>xattr_handles. Should I push it to you and wait until it works it way

>into Greg's tree. What do the merge schedules look like. Lastly the

>a_refcount for the POSIX acl changed with your xattr updates which now

>causes lustre to crash :-(

> 

>> Thanks,

>> Andreas

>> 

>>  drivers/staging/lustre/lustre/llite/Makefile       |  1 +

>>  .../staging/lustre/lustre/llite/llite_internal.h   |  3 ++

>>  drivers/staging/lustre/lustre/llite/posix_acl.c    | 62 ++++++++++++++++++++++

>>  drivers/staging/lustre/lustre/llite/xattr.c        |  8 ++-

>>  fs/cifs/cifssmb.c                                  | 41 ++++++++++----

>>  fs/posix_acl.c                                     | 62 +---------------------

>>  fs/xattr.c                                         |  7 ---

>>  include/linux/posix_acl_xattr.h                    | 12 -----

>>  8 files changed, 107 insertions(+), 89 deletions(-)

>>  create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c

>> 

>> diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile

>> index 2ce10ff..67125f8 100644

>> --- a/drivers/staging/lustre/lustre/llite/Makefile

>> +++ b/drivers/staging/lustre/lustre/llite/Makefile

>> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \

>>  	    glimpse.o lcommon_cl.o lcommon_misc.o \

>>  	    vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \

>>  	    lproc_llite.o

>> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o

>>  

>>  llite_lloop-y := lloop.o

>> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h

>> index ce1f949..d454dfb 100644

>> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h

>> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h

>> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);

>>  __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);

>>  __u32 cl_fid_build_gen(const struct lu_fid *fid);

>>  

>> +void posix_acl_fix_xattr_from_user(void *value, size_t size);

>> +void posix_acl_fix_xattr_to_user(void *value, size_t size);

>> +

>>  #endif /* LLITE_INTERNAL_H */

>> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c

>> new file mode 100644

>> index 0000000..4fabd0f

>> --- /dev/null

>> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c

>> @@ -0,0 +1,62 @@

>> +#include <linux/kernel.h>

>> +#include <linux/fs.h>

>> +#include <linux/posix_acl_xattr.h>

>> +#include <linux/user_namespace.h>

>> +

>> +/*

>> + * Fix up the uids and gids in posix acl extended attributes in place.

>> + */

>> +static void posix_acl_fix_xattr_userns(

>> +	struct user_namespace *to, struct user_namespace *from,

>> +	void *value, size_t size)

>> +{

>> +	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;

>> +	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;

>> +	int count;

>> +	kuid_t uid;

>> +	kgid_t gid;

>> +

>> +	if (!value)

>> +		return;

>> +	if (size < sizeof(posix_acl_xattr_header))

>> +		return;

>> +	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))

>> +		return;

>> +

>> +	count = posix_acl_xattr_count(size);

>> +	if (count < 0)

>> +		return;

>> +	if (count == 0)

>> +		return;

>> +

>> +	for (end = entry + count; entry != end; entry++) {

>> +		switch(le16_to_cpu(entry->e_tag)) {

>> +		case ACL_USER:

>> +			uid = make_kuid(from, le32_to_cpu(entry->e_id));

>> +			entry->e_id = cpu_to_le32(from_kuid(to, uid));

>> +			break;

>> +		case ACL_GROUP:

>> +			gid = make_kgid(from, le32_to_cpu(entry->e_id));

>> +			entry->e_id = cpu_to_le32(from_kgid(to, gid));

>> +			break;

>> +		default:

>> +			break;

>> +		}

>> +	}

>> +}

>> +

>> +void posix_acl_fix_xattr_from_user(void *value, size_t size)

>> +{

>> +	struct user_namespace *user_ns = current_user_ns();

>> +	if (user_ns == &init_user_ns)

>> +		return;

>> +	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);

>> +}

>> +

>> +void posix_acl_fix_xattr_to_user(void *value, size_t size)

>> +{

>> +	struct user_namespace *user_ns = current_user_ns();

>> +	if (user_ns == &init_user_ns)

>> +		return;

>> +	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);

>> +}

>> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c

>> index ed4de04..c721b44 100644

>> --- a/drivers/staging/lustre/lustre/llite/xattr.c

>> +++ b/drivers/staging/lustre/lustre/llite/xattr.c

>> @@ -144,6 +144,9 @@ int ll_setxattr_common(struct inode *inode, const char *name,

>>  		return -EOPNOTSUPP;

>>  

>>  #ifdef CONFIG_FS_POSIX_ACL

>> +	if (xattr_type == XATTR_ACL_ACCESS_T ||

>> +	    xattr_type == XATTR_ACL_DEFAULT_T)

>> +		posix_acl_fix_xattr_from_user((void *)value, size);

>>  	if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&

>>  	    (xattr_type == XATTR_ACL_ACCESS_T ||

>>  	    xattr_type == XATTR_ACL_DEFAULT_T)) {

>> @@ -348,7 +351,7 @@ int ll_getxattr_common(struct inode *inode, const char *name,

>>  		if (!acl)

>>  			return -ENODATA;

>>  

>> -		rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);

>> +		rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);

>>  		posix_acl_release(acl);

>>  		return rc;

>>  	}

>> @@ -436,6 +439,9 @@ getxattr_nocache:

>>  			goto out;

>>  		}

>>  	}

>> +	if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||

>> +			xattr_type == XATTR_ACL_DEFAULT_T))

>> +		posix_acl_fix_xattr_to_user(buffer, rc);

>>  #endif

>>  

>>  out_xattr:

>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c

>> index d47197e..9dc001f 100644

>> --- a/fs/cifs/cifssmb.c

>> +++ b/fs/cifs/cifssmb.c

>> @@ -3337,10 +3337,25 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,

>>  static void cifs_convert_ace(posix_acl_xattr_entry *ace,

>>  			     struct cifs_posix_ace *cifs_ace)

>>  {

>> +	u32 cifs_id, id = -1;

>> +

>>  	/* u8 cifs fields do not need le conversion */

>>  	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);

>>  	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);

>> -	ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));

>> +	switch(cifs_ace->cifs_e_tag) {

>> +	case ACL_USER:

>> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);

>> +		id = from_kuid(current_user_ns(),

>> +			       make_kuid(&init_user_ns, cifs_id));

>> +		break;

>> +

>> +	case ACL_GROUP:

>> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);

>> +		id = from_kgid(current_user_ns(),

>> +			       make_kgid(&init_user_ns, cifs_id));

>> +		break;

>> +	}

>> +	ace->e_id = cpu_to_le32(id);

>>  /*

>>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",

>>  		 ace->e_perm, ace->e_tag, ace->e_id);

>> @@ -3408,21 +3423,29 @@ static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,

>>  static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,

>>  				     const posix_acl_xattr_entry *local_ace)

>>  {

>> -	__u16 rc = 0; /* 0 = ACL converted ok */

>> +	u32 cifs_id = -1, id;

>>  

>>  	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);

>>  	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);

>> -	/* BB is there a better way to handle the large uid? */

>> -	if (local_ace->e_id == cpu_to_le32(-1)) {

>> -	/* Probably no need to le convert -1 on any arch but can not hurt */

>> -		cifs_ace->cifs_uid = cpu_to_le64(-1);

>> -	} else

>> -		cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));

>> +	switch(cifs_ace->cifs_e_tag) {

>> +	case ACL_USER:

>> +		id = le32_to_cpu(local_ace->e_id);

>> +		cifs_id = from_kuid(&init_user_ns,

>> +				    make_kuid(current_user_ns(), id));

>> +		break;

>> +

>> +	case ACL_GROUP:

>> +		id = le32_to_cpu(local_ace->e_id);

>> +		cifs_id = from_kgid(&init_user_ns,

>> +				    make_kgid(current_user_ns(), id));

>> +		break;

>> +	}

>> +	cifs_ace->cifs_uid = cpu_to_le64(cifs_id);

>>  /*

>>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",

>>  		 ace->e_perm, ace->e_tag, ace->e_id);

>>  */

>> -	return rc;

>> +	return 0;

>>  }

>>  

>>  /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */

>> diff --git a/fs/posix_acl.c b/fs/posix_acl.c

>> index 2c60f17..fca281c 100644

>> --- a/fs/posix_acl.c

>> +++ b/fs/posix_acl.c

>> @@ -627,64 +627,6 @@ no_mem:

>>  EXPORT_SYMBOL_GPL(posix_acl_create);

>>  

>>  /*

>> - * Fix up the uids and gids in posix acl extended attributes in place.

>> - */

>> -static void posix_acl_fix_xattr_userns(

>> -	struct user_namespace *to, struct user_namespace *from,

>> -	void *value, size_t size)

>> -{

>> -	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;

>> -	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;

>> -	int count;

>> -	kuid_t uid;

>> -	kgid_t gid;

>> -

>> -	if (!value)

>> -		return;

>> -	if (size < sizeof(posix_acl_xattr_header))

>> -		return;

>> -	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))

>> -		return;

>> -

>> -	count = posix_acl_xattr_count(size);

>> -	if (count < 0)

>> -		return;

>> -	if (count == 0)

>> -		return;

>> -

>> -	for (end = entry + count; entry != end; entry++) {

>> -		switch(le16_to_cpu(entry->e_tag)) {

>> -		case ACL_USER:

>> -			uid = make_kuid(from, le32_to_cpu(entry->e_id));

>> -			entry->e_id = cpu_to_le32(from_kuid(to, uid));

>> -			break;

>> -		case ACL_GROUP:

>> -			gid = make_kgid(from, le32_to_cpu(entry->e_id));

>> -			entry->e_id = cpu_to_le32(from_kgid(to, gid));

>> -			break;

>> -		default:

>> -			break;

>> -		}

>> -	}

>> -}

>> -

>> -void posix_acl_fix_xattr_from_user(void *value, size_t size)

>> -{

>> -	struct user_namespace *user_ns = current_user_ns();

>> -	if (user_ns == &init_user_ns)

>> -		return;

>> -	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);

>> -}

>> -

>> -void posix_acl_fix_xattr_to_user(void *value, size_t size)

>> -{

>> -	struct user_namespace *user_ns = current_user_ns();

>> -	if (user_ns == &init_user_ns)

>> -		return;

>> -	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);

>> -}

>> -

>> -/*

>>   * Convert from extended attribute to in-memory representation.

>>   */

>>  struct posix_acl *

>> @@ -814,7 +756,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,

>>  	if (acl == NULL)

>>  		return -ENODATA;

>>  

>> -	error = posix_acl_to_xattr(&init_user_ns, acl, value, size);

>> +	error = posix_acl_to_xattr(current_user_ns(), acl, value, size);

>>  	posix_acl_release(acl);

>>  

>>  	return error;

>> @@ -840,7 +782,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,

>>  		return -EPERM;

>>  

>>  	if (value) {

>> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);

>> +		acl = posix_acl_from_xattr(current_user_ns(), value, size);

>>  		if (IS_ERR(acl))

>>  			return PTR_ERR(acl);

>>  

>> diff --git a/fs/xattr.c b/fs/xattr.c

>> index b11945e..b648b05 100644

>> --- a/fs/xattr.c

>> +++ b/fs/xattr.c

>> @@ -20,7 +20,6 @@

>>  #include <linux/fsnotify.h>

>>  #include <linux/audit.h>

>>  #include <linux/vmalloc.h>

>> -#include <linux/posix_acl_xattr.h>

>>  

>>  #include <asm/uaccess.h>

>>  

>> @@ -329,9 +328,6 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,

>>  			error = -EFAULT;

>>  			goto out;

>>  		}

>> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||

>> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))

>> -			posix_acl_fix_xattr_from_user(kvalue, size);

>>  	}

>>  

>>  	error = vfs_setxattr(d, kname, kvalue, size, flags);

>> @@ -426,9 +422,6 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,

>>  

>>  	error = vfs_getxattr(d, kname, kvalue, size);

>>  	if (error > 0) {

>> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||

>> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))

>> -			posix_acl_fix_xattr_to_user(kvalue, size);

>>  		if (size && copy_to_user(value, kvalue, error))

>>  			error = -EFAULT;

>>  	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {

>> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h

>> index e5e8ec4..9789aba 100644

>> --- a/include/linux/posix_acl_xattr.h

>> +++ b/include/linux/posix_acl_xattr.h

>> @@ -48,18 +48,6 @@ posix_acl_xattr_count(size_t size)

>>  	return size / sizeof(posix_acl_xattr_entry);

>>  }

>>  

>> -#ifdef CONFIG_FS_POSIX_ACL

>> -void posix_acl_fix_xattr_from_user(void *value, size_t size);

>> -void posix_acl_fix_xattr_to_user(void *value, size_t size);

>> -#else

>> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)

>> -{

>> -}

>> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)

>> -{

>> -}

>> -#endif

>> -

>>  struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns, 

>>  				       const void *value, size_t size);

>>  int posix_acl_to_xattr(struct user_namespace *user_ns,

>> -- 

>> 2.5.5

>> 

>> _______________________________________________

>> lustre-devel mailing list

>> lustre-devel@lists.lustre.org

>> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

>> 

>_______________________________________________

>lustre-devel mailing list

>lustre-devel@lists.lustre.org

>http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

>
James Simmons May 24, 2016, 8:35 p.m. UTC | #8
> On 2016/05/23, 15:06, "James Simmons" <jsimmons@infradead.org> wrote:
> >
> >> Currently, getxattr() and setxattr() check for the xattr names
> >> "system.posix_acl_{access,default}" and perform in-place UID / GID
> >> namespace mappings in the xattr values. Filesystems then again check for
> >> the same xattr names to handle those attributes, almost always using the
> >> standard posix_acl_{access,default}_xattr_handler handlers.  This is
> >> unnecessary overhead; move the namespace conversion into the xattr
> >> handlers instead.
> >> 
> >> For filesystems that use the POSIX ACL xattr handlers, no change is
> >> required.  Filesystems that don't use those handlers (cifs and lustre)
> >> need to take care of the namespace mapping themselves now.
> >> 
> >> The only user left of the posix_acl_fix_xattr_{from,to}_user helpers is
> >> lustre, so this patch moves them into lustre.
> >> 
> >> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> >> ---
> >> 
> >> I'm reasonably confident about the core and cifs changes in this patch.
> >> The lustre code is pretty weird though, so could I please get a careful
> >> review on the changes there?
> >
> >Nak on the Lustre changes. POSIX ACLs are also handled in mdc_request.c.
> >Besides POSIX ACLs lustre has created a extended ACL as well that is 
> >grouped in with posix ACL handling. This extended ACL is like POSIX acls 
> >except it also contains the state (deleted, modified, ...) of the ACL. 
> >Besides normal local access control list handling Lustre manages remote
> >access control list handling. You can read about this handling is in 
> >llite_rmtacl.c. This code was written long before I became involved with
> >lustre so the exact details are fuzzy to me.
> 
> James,
> the remote ACL code never found any usage in the field and can be
> deleted.  Please see http://review.whamcloud.com/19789 for details.

That is a huge cleanup which will make Greg very happy. The tools and test
are going to be updated so the landing has to be just right so we 
don't have a flood of test failures. Gruenbacher with the 19789 patch 
Dilger pointed out Lustre's POSIX ACL code just becomes ordinary which
means we can use the default POSIS xattr handler. You wouldn't have to
keep posix_acl.c around with these changes.

> Cheers, Andreas
> 
> > The guts of this are handled is located at:
> >
> >drivers/staging/lustre/lustre/obdclass/acl.c
> >
> >P.S
> >
> >    A you probably have noticed Lustre has had an uptick in development
> >which means the code is now changing all the time in staging. How should
> >we handle the changes you are working in your own trees verses what is
> >happing in staging. For example I'm looking at moving lustre to the
> >xattr_handles. Should I push it to you and wait until it works it way
> >into Greg's tree. What do the merge schedules look like. Lastly the
> >a_refcount for the POSIX acl changed with your xattr updates which now
> >causes lustre to crash :-(
> > 
> >> Thanks,
> >> Andreas
> >> 
> >>  drivers/staging/lustre/lustre/llite/Makefile       |  1 +
> >>  .../staging/lustre/lustre/llite/llite_internal.h   |  3 ++
> >>  drivers/staging/lustre/lustre/llite/posix_acl.c    | 62 ++++++++++++++++++++++
> >>  drivers/staging/lustre/lustre/llite/xattr.c        |  8 ++-
> >>  fs/cifs/cifssmb.c                                  | 41 ++++++++++----
> >>  fs/posix_acl.c                                     | 62 +---------------------
> >>  fs/xattr.c                                         |  7 ---
> >>  include/linux/posix_acl_xattr.h                    | 12 -----
> >>  8 files changed, 107 insertions(+), 89 deletions(-)
> >>  create mode 100644 drivers/staging/lustre/lustre/llite/posix_acl.c
> >> 
> >> diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile
> >> index 2ce10ff..67125f8 100644
> >> --- a/drivers/staging/lustre/lustre/llite/Makefile
> >> +++ b/drivers/staging/lustre/lustre/llite/Makefile
> >> @@ -7,5 +7,6 @@ lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \
> >>  	    glimpse.o lcommon_cl.o lcommon_misc.o \
> >>  	    vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
> >>  	    lproc_llite.o
> >> +lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
> >>  
> >>  llite_lloop-y := lloop.o
> >> diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> index ce1f949..d454dfb 100644
> >> --- a/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> +++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
> >> @@ -1402,4 +1402,7 @@ int cl_local_size(struct inode *inode);
> >>  __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
> >>  __u32 cl_fid_build_gen(const struct lu_fid *fid);
> >>  
> >> +void posix_acl_fix_xattr_from_user(void *value, size_t size);
> >> +void posix_acl_fix_xattr_to_user(void *value, size_t size);
> >> +
> >>  #endif /* LLITE_INTERNAL_H */
> >> diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c
> >> new file mode 100644
> >> index 0000000..4fabd0f
> >> --- /dev/null
> >> +++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
> >> @@ -0,0 +1,62 @@
> >> +#include <linux/kernel.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/posix_acl_xattr.h>
> >> +#include <linux/user_namespace.h>
> >> +
> >> +/*
> >> + * Fix up the uids and gids in posix acl extended attributes in place.
> >> + */
> >> +static void posix_acl_fix_xattr_userns(
> >> +	struct user_namespace *to, struct user_namespace *from,
> >> +	void *value, size_t size)
> >> +{
> >> +	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> >> +	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> >> +	int count;
> >> +	kuid_t uid;
> >> +	kgid_t gid;
> >> +
> >> +	if (!value)
> >> +		return;
> >> +	if (size < sizeof(posix_acl_xattr_header))
> >> +		return;
> >> +	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> >> +		return;
> >> +
> >> +	count = posix_acl_xattr_count(size);
> >> +	if (count < 0)
> >> +		return;
> >> +	if (count == 0)
> >> +		return;
> >> +
> >> +	for (end = entry + count; entry != end; entry++) {
> >> +		switch(le16_to_cpu(entry->e_tag)) {
> >> +		case ACL_USER:
> >> +			uid = make_kuid(from, le32_to_cpu(entry->e_id));
> >> +			entry->e_id = cpu_to_le32(from_kuid(to, uid));
> >> +			break;
> >> +		case ACL_GROUP:
> >> +			gid = make_kgid(from, le32_to_cpu(entry->e_id));
> >> +			entry->e_id = cpu_to_le32(from_kgid(to, gid));
> >> +			break;
> >> +		default:
> >> +			break;
> >> +		}
> >> +	}
> >> +}
> >> +
> >> +void posix_acl_fix_xattr_from_user(void *value, size_t size)
> >> +{
> >> +	struct user_namespace *user_ns = current_user_ns();
> >> +	if (user_ns == &init_user_ns)
> >> +		return;
> >> +	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> >> +}
> >> +
> >> +void posix_acl_fix_xattr_to_user(void *value, size_t size)
> >> +{
> >> +	struct user_namespace *user_ns = current_user_ns();
> >> +	if (user_ns == &init_user_ns)
> >> +		return;
> >> +	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> >> +}
> >> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> >> index ed4de04..c721b44 100644
> >> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> >> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> >> @@ -144,6 +144,9 @@ int ll_setxattr_common(struct inode *inode, const char *name,
> >>  		return -EOPNOTSUPP;
> >>  
> >>  #ifdef CONFIG_FS_POSIX_ACL
> >> +	if (xattr_type == XATTR_ACL_ACCESS_T ||
> >> +	    xattr_type == XATTR_ACL_DEFAULT_T)
> >> +		posix_acl_fix_xattr_from_user((void *)value, size);
> >>  	if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&
> >>  	    (xattr_type == XATTR_ACL_ACCESS_T ||
> >>  	    xattr_type == XATTR_ACL_DEFAULT_T)) {
> >> @@ -348,7 +351,7 @@ int ll_getxattr_common(struct inode *inode, const char *name,
> >>  		if (!acl)
> >>  			return -ENODATA;
> >>  
> >> -		rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> >> +		rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);
> >>  		posix_acl_release(acl);
> >>  		return rc;
> >>  	}
> >> @@ -436,6 +439,9 @@ getxattr_nocache:
> >>  			goto out;
> >>  		}
> >>  	}
> >> +	if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||
> >> +			xattr_type == XATTR_ACL_DEFAULT_T))
> >> +		posix_acl_fix_xattr_to_user(buffer, rc);
> >>  #endif
> >>  
> >>  out_xattr:
> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >> index d47197e..9dc001f 100644
> >> --- a/fs/cifs/cifssmb.c
> >> +++ b/fs/cifs/cifssmb.c
> >> @@ -3337,10 +3337,25 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> >>  static void cifs_convert_ace(posix_acl_xattr_entry *ace,
> >>  			     struct cifs_posix_ace *cifs_ace)
> >>  {
> >> +	u32 cifs_id, id = -1;
> >> +
> >>  	/* u8 cifs fields do not need le conversion */
> >>  	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
> >>  	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
> >> -	ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
> >> +	switch(cifs_ace->cifs_e_tag) {
> >> +	case ACL_USER:
> >> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> >> +		id = from_kuid(current_user_ns(),
> >> +			       make_kuid(&init_user_ns, cifs_id));
> >> +		break;
> >> +
> >> +	case ACL_GROUP:
> >> +		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
> >> +		id = from_kgid(current_user_ns(),
> >> +			       make_kgid(&init_user_ns, cifs_id));
> >> +		break;
> >> +	}
> >> +	ace->e_id = cpu_to_le32(id);
> >>  /*
> >>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",
> >>  		 ace->e_perm, ace->e_tag, ace->e_id);
> >> @@ -3408,21 +3423,29 @@ static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
> >>  static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
> >>  				     const posix_acl_xattr_entry *local_ace)
> >>  {
> >> -	__u16 rc = 0; /* 0 = ACL converted ok */
> >> +	u32 cifs_id = -1, id;
> >>  
> >>  	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
> >>  	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
> >> -	/* BB is there a better way to handle the large uid? */
> >> -	if (local_ace->e_id == cpu_to_le32(-1)) {
> >> -	/* Probably no need to le convert -1 on any arch but can not hurt */
> >> -		cifs_ace->cifs_uid = cpu_to_le64(-1);
> >> -	} else
> >> -		cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
> >> +	switch(cifs_ace->cifs_e_tag) {
> >> +	case ACL_USER:
> >> +		id = le32_to_cpu(local_ace->e_id);
> >> +		cifs_id = from_kuid(&init_user_ns,
> >> +				    make_kuid(current_user_ns(), id));
> >> +		break;
> >> +
> >> +	case ACL_GROUP:
> >> +		id = le32_to_cpu(local_ace->e_id);
> >> +		cifs_id = from_kgid(&init_user_ns,
> >> +				    make_kgid(current_user_ns(), id));
> >> +		break;
> >> +	}
> >> +	cifs_ace->cifs_uid = cpu_to_le64(cifs_id);
> >>  /*
> >>  	cifs_dbg(FYI, "perm %d tag %d id %d\n",
> >>  		 ace->e_perm, ace->e_tag, ace->e_id);
> >>  */
> >> -	return rc;
> >> +	return 0;
> >>  }
> >>  
> >>  /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
> >> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> >> index 2c60f17..fca281c 100644
> >> --- a/fs/posix_acl.c
> >> +++ b/fs/posix_acl.c
> >> @@ -627,64 +627,6 @@ no_mem:
> >>  EXPORT_SYMBOL_GPL(posix_acl_create);
> >>  
> >>  /*
> >> - * Fix up the uids and gids in posix acl extended attributes in place.
> >> - */
> >> -static void posix_acl_fix_xattr_userns(
> >> -	struct user_namespace *to, struct user_namespace *from,
> >> -	void *value, size_t size)
> >> -{
> >> -	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> >> -	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> >> -	int count;
> >> -	kuid_t uid;
> >> -	kgid_t gid;
> >> -
> >> -	if (!value)
> >> -		return;
> >> -	if (size < sizeof(posix_acl_xattr_header))
> >> -		return;
> >> -	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> >> -		return;
> >> -
> >> -	count = posix_acl_xattr_count(size);
> >> -	if (count < 0)
> >> -		return;
> >> -	if (count == 0)
> >> -		return;
> >> -
> >> -	for (end = entry + count; entry != end; entry++) {
> >> -		switch(le16_to_cpu(entry->e_tag)) {
> >> -		case ACL_USER:
> >> -			uid = make_kuid(from, le32_to_cpu(entry->e_id));
> >> -			entry->e_id = cpu_to_le32(from_kuid(to, uid));
> >> -			break;
> >> -		case ACL_GROUP:
> >> -			gid = make_kgid(from, le32_to_cpu(entry->e_id));
> >> -			entry->e_id = cpu_to_le32(from_kgid(to, gid));
> >> -			break;
> >> -		default:
> >> -			break;
> >> -		}
> >> -	}
> >> -}
> >> -
> >> -void posix_acl_fix_xattr_from_user(void *value, size_t size)
> >> -{
> >> -	struct user_namespace *user_ns = current_user_ns();
> >> -	if (user_ns == &init_user_ns)
> >> -		return;
> >> -	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> >> -}
> >> -
> >> -void posix_acl_fix_xattr_to_user(void *value, size_t size)
> >> -{
> >> -	struct user_namespace *user_ns = current_user_ns();
> >> -	if (user_ns == &init_user_ns)
> >> -		return;
> >> -	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> >> -}
> >> -
> >> -/*
> >>   * Convert from extended attribute to in-memory representation.
> >>   */
> >>  struct posix_acl *
> >> @@ -814,7 +756,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,
> >>  	if (acl == NULL)
> >>  		return -ENODATA;
> >>  
> >> -	error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> >> +	error = posix_acl_to_xattr(current_user_ns(), acl, value, size);
> >>  	posix_acl_release(acl);
> >>  
> >>  	return error;
> >> @@ -840,7 +782,7 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
> >>  		return -EPERM;
> >>  
> >>  	if (value) {
> >> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> >> +		acl = posix_acl_from_xattr(current_user_ns(), value, size);
> >>  		if (IS_ERR(acl))
> >>  			return PTR_ERR(acl);
> >>  
> >> diff --git a/fs/xattr.c b/fs/xattr.c
> >> index b11945e..b648b05 100644
> >> --- a/fs/xattr.c
> >> +++ b/fs/xattr.c
> >> @@ -20,7 +20,6 @@
> >>  #include <linux/fsnotify.h>
> >>  #include <linux/audit.h>
> >>  #include <linux/vmalloc.h>
> >> -#include <linux/posix_acl_xattr.h>
> >>  
> >>  #include <asm/uaccess.h>
> >>  
> >> @@ -329,9 +328,6 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
> >>  			error = -EFAULT;
> >>  			goto out;
> >>  		}
> >> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> >> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> >> -			posix_acl_fix_xattr_from_user(kvalue, size);
> >>  	}
> >>  
> >>  	error = vfs_setxattr(d, kname, kvalue, size, flags);
> >> @@ -426,9 +422,6 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
> >>  
> >>  	error = vfs_getxattr(d, kname, kvalue, size);
> >>  	if (error > 0) {
> >> -		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> >> -		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> >> -			posix_acl_fix_xattr_to_user(kvalue, size);
> >>  		if (size && copy_to_user(value, kvalue, error))
> >>  			error = -EFAULT;
> >>  	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> >> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> >> index e5e8ec4..9789aba 100644
> >> --- a/include/linux/posix_acl_xattr.h
> >> +++ b/include/linux/posix_acl_xattr.h
> >> @@ -48,18 +48,6 @@ posix_acl_xattr_count(size_t size)
> >>  	return size / sizeof(posix_acl_xattr_entry);
> >>  }
> >>  
> >> -#ifdef CONFIG_FS_POSIX_ACL
> >> -void posix_acl_fix_xattr_from_user(void *value, size_t size);
> >> -void posix_acl_fix_xattr_to_user(void *value, size_t size);
> >> -#else
> >> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
> >> -{
> >> -}
> >> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
> >> -{
> >> -}
> >> -#endif
> >> -
> >>  struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns, 
> >>  				       const void *value, size_t size);
> >>  int posix_acl_to_xattr(struct user_namespace *user_ns,
> >> -- 
> >> 2.5.5
> >> 
> >> _______________________________________________
> >> lustre-devel mailing list
> >> lustre-devel@lists.lustre.org
> >> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
> >> 
> >_______________________________________________
> >lustre-devel mailing list
> >lustre-devel@lists.lustre.org
> >http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
> >
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger May 24, 2016, 8:47 p.m. UTC | #9
On May 24, 2016, at 2:47 AM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
>> I'm reasonably confident about the core and cifs changes in this patch.
>> The lustre code is pretty weird though, so could I please get a careful
>> review on the changes there?
> 
> Lustre is in the staging tree because it's an unrescuable piece of junk.
> Please just ignore it and move the proper parts of the kernel forward.

Don't be shy Christoph, tell us how you really feel about Lustre... :-)


Fortunately, that "piece of junk" is working fine and is running on most of
the supercomputers in the world (9 of the top 10, 70%+ of the top 100, ...)
with several sites having 50PB+ of storage, and 1TB/s+ to 20000+ clients,
in a single coherent POSIX filesystem.

It's true that the staging client isn't getting cleaned up as fast as
anyone likes, but that is because it is complex software in use at many
sites for many years and we can't just stop to completely rewrite the
code and hope it still works.  Even XFS took many years to get rid of the
IRIX wrapping layers, and still does a lot of things outside of the VFS/VM
(stats, memory allocation, IO submission, etc.) because the VFS/VM doesn't
work the way XFS wants it to (or vice versa).  This is not a slight against
XFS, just a sign that non-trivial filesystems take a long time to change
while maintaining existing functionality.  This is mainly a difference in
how code is added to the kernel today vs. how it was added back then.


Cheers, Andreas
Andreas Gruenbacher May 24, 2016, 10:40 p.m. UTC | #10
On Tue, May 24, 2016 at 10:35 PM, James Simmons <jsimmons@infradead.org> wrote:
> That is a huge cleanup which will make Greg very happy. The tools and test
> are going to be updated so the landing has to be just right so we
> don't have a flood of test failures. Gruenbacher with the 19789 patch
> Dilger pointed out Lustre's POSIX ACL code just becomes ordinary which
> means we can use the default POSIS xattr handler. You wouldn't have to
> keep posix_acl.c around with these changes.

Change http://review.whamcloud.com/19789 doesn't switch Lustre over to
using xattr handlers. As long as Lustre doesn't use those (i.e., use
posix_acl_access_xattr_handler and posix_acl_default_xattr_handler),
it will still need the uid/gid namespace conversion code in ll_getxattr and
ll_setxattr for this patch to be correct.

The merge conflict between this patch and http://review.whamcloud.com/19789
is easily resolved though.

Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman May 25, 2016, 11:30 p.m. UTC | #11
Andreas Gruenbacher <agruenba@redhat.com> writes:

> On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
>>> Currently, getxattr() and setxattr() check for the xattr names
>>> "system.posix_acl_{access,default}" and perform in-place UID / GID
>>> namespace mappings in the xattr values. Filesystems then again check for
>>> the same xattr names to handle those attributes, almost always using the
>>> standard posix_acl_{access,default}_xattr_handler handlers.  This is
>>> unnecessary overhead; move the namespace conversion into the xattr
>>> handlers instead.
>>
>> Please, are you sure that the changes in posix_acl_xattr_get() and
>> posix_acl_xattr_set() are safe ? you are reading into current user
>> namespace, from a first view this is not safe unless I'm missing
>> something... they should map into init_user_ns...
>
> Yes, moving the namespace conversion from the VFS into those functions
> so that we don't have to check for those attributes and parse them
> twice is exactly the point of this patch.

In general I am in favor of cleaning up the xattr and acl code in the
kernel.  However I am not certain that your changes succeed in getting
us where we want to go.

My feel is that what we want to do is to update the cached acl when we
write it from userspace, to update the disk with the new acl when the
inode is sync'd to disk, and let the vfs handle the translation from
the cached posix acl to the vfs getxattr and setxattr system calls.
In the long term anything else is madness.

Currently posix acl reads and updates bypass the vfs acl cache for the
inode and that just looks wrong.


The reason that fixup happens in a separate pass from everything else
today is that when I was wrote posix_acl_fix_xattr_to_user and
posix_acl_xattr_from_user a number of filesystems had a very strange
structure that completely bypassed any code conversion routines and
made some strange assumptions.  I don't remember which filesystems those
were but it was neither lustre, nor cifs, nor nfs that were the problem
cases when I was looking.  I don't see your patch  addressing that
problem so either someone has already fixed those issues or they have
been overlooked.


There is a complication that is comming shortly (next merge window
unless major unfixable bugs show up between now and them).  There is a
new field s_user_ns that is being introduced for filesystems to record
their owner and their default translation rules to kuids.  That will
make the hard coded &init_user_ns values in your patch wrong.

>> Please Cc the user namespace maintainers before. Thank you!
>
> Eric, Andy, anyone else?

Serge Hallyn has a pending patch that adds a similar translation to
the security.capability xattr.  Which is one more case of where caching
and translation at the VFS layer are makes sense.

All of that said I am definitely in favor of cleaning up this area of
code.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher May 26, 2016, 11:36 a.m. UTC | #12
On Thu, May 26, 2016 at 1:30 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andreas Gruenbacher <agruenba@redhat.com> writes:
>
>> On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
>>>> Currently, getxattr() and setxattr() check for the xattr names
>>>> "system.posix_acl_{access,default}" and perform in-place UID / GID
>>>> namespace mappings in the xattr values. Filesystems then again check for
>>>> the same xattr names to handle those attributes, almost always using the
>>>> standard posix_acl_{access,default}_xattr_handler handlers.  This is
>>>> unnecessary overhead; move the namespace conversion into the xattr
>>>> handlers instead.
>>>
>>> Please, are you sure that the changes in posix_acl_xattr_get() and
>>> posix_acl_xattr_set() are safe ? you are reading into current user
>>> namespace, from a first view this is not safe unless I'm missing
>>> something... they should map into init_user_ns...
>>
>> Yes, moving the namespace conversion from the VFS into those functions
>> so that we don't have to check for those attributes and parse them
>> twice is exactly the point of this patch.
>
> In general I am in favor of cleaning up the xattr and acl code in the
> kernel.  However I am not certain that your changes succeed in getting
> us where we want to go.
>
> My feel is that what we want to do is to update the cached acl when we
> write it from userspace, to update the disk with the new acl when the
> inode is sync'd to disk, and let the vfs handle the translation from
> the cached posix acl to the vfs getxattr and setxattr system calls.
> In the long term anything else is madness.
>
> Currently posix acl reads and updates bypass the vfs acl cache for the
> inode and that just looks wrong.

Not all filesystems cache acls in the inode (i_acl and i_default_acl), so there
has to be some way to "bypass the cache" if you want to put it that way.

All normal filesystems that cache ACLs locally implement the get_acl and
set_acl inode operations. They put posix_acl_access_xattr_handler
and posix_acl_default_xattr_handler into s_xattr and use the
generic_{get,set,remove}xattr inode operations to hook things up
appropriately. The xattr handlers convert to/from xattrs and struct posix_acl
and call the get_acl and set_acl inode operations. That's where the namespace
conversion is supposed to happen as well; this avoids special-casing
it in the VFS.

The only filesystems that don't do things that way are CIFS and Lustre. Doing
the appropriate namespace mapping in CIFS is easy (see this patch). So it's only
Lustre that's left with the original in-place xattr conversion.

Another pending cleanup (https://lwn.net/Articles/688390/) gets rid of the
{get,set,remove}xattr inode operations in favor of using s_xattr directly.

> The reason that fixup happens in a separate pass from everything else
> today is that when I wrote posix_acl_fix_xattr_to_user and
> posix_acl_xattr_from_user a number of filesystems had a very strange
> structure that completely bypassed any code conversion routines and
> made some strange assumptions.  I don't remember which filesystems those
> were but it was neither lustre, nor cifs, nor nfs that were the problem
> cases when I was looking.  I don't see your patch  addressing that
> problem so either someone has already fixed those issues or they have
> been overlooked.

There have been cleanups in this area before, so I assume things were
fixed. In any case, I didn't blindly hack this together, I've actually checked
all the filesystems.

> There is a complication that is comming shortly (next merge window
> unless major unfixable bugs show up between now and them).  There is a
> new field s_user_ns that is being introduced for filesystems to record
> their owner and their default translation rules to kuids.  That will
> make the hard coded &init_user_ns values in your patch wrong.

Okay, that conflict should be easy enough to resolve. Where's that code?

>>> Please Cc the user namespace maintainers before. Thank you!
>>
>> Eric, Andy, anyone else?
>
> Serge Hallyn has a pending patch that adds a similar translation to
> the security.capability xattr.  Which is one more case of where caching
> and translation at the VFS layer are makes sense.

No. Just like with this patch, that mapping really needs to go into the
appropriate xattr handler. Again, where's that code, please?

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman May 27, 2016, 6:07 p.m. UTC | #13
Andreas Gruenbacher <agruenba@redhat.com> writes:

> On Thu, May 26, 2016 at 1:30 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Andreas Gruenbacher <agruenba@redhat.com> writes:
>>
>>> On Tue, May 24, 2016 at 5:41 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>>> On Mon, May 23, 2016 at 03:09:49PM +0200, Andreas Gruenbacher wrote:
>>>>> Currently, getxattr() and setxattr() check for the xattr names
>>>>> "system.posix_acl_{access,default}" and perform in-place UID / GID
>>>>> namespace mappings in the xattr values. Filesystems then again check for
>>>>> the same xattr names to handle those attributes, almost always using the
>>>>> standard posix_acl_{access,default}_xattr_handler handlers.  This is
>>>>> unnecessary overhead; move the namespace conversion into the xattr
>>>>> handlers instead.
>>>>
>>>> Please, are you sure that the changes in posix_acl_xattr_get() and
>>>> posix_acl_xattr_set() are safe ? you are reading into current user
>>>> namespace, from a first view this is not safe unless I'm missing
>>>> something... they should map into init_user_ns...
>>>
>>> Yes, moving the namespace conversion from the VFS into those functions
>>> so that we don't have to check for those attributes and parse them
>>> twice is exactly the point of this patch.
>>
>> In general I am in favor of cleaning up the xattr and acl code in the
>> kernel.  However I am not certain that your changes succeed in getting
>> us where we want to go.
>>
>> My feel is that what we want to do is to update the cached acl when we
>> write it from userspace, to update the disk with the new acl when the
>> inode is sync'd to disk, and let the vfs handle the translation from
>> the cached posix acl to the vfs getxattr and setxattr system calls.
>> In the long term anything else is madness.
>>
>> Currently posix acl reads and updates bypass the vfs acl cache for the
>> inode and that just looks wrong.
>
> Not all filesystems cache acls in the inode (i_acl and i_default_acl), so there
> has to be some way to "bypass the cache" if you want to put it that way.
>
> All normal filesystems that cache ACLs locally implement the get_acl and
> set_acl inode operations. They put posix_acl_access_xattr_handler
> and posix_acl_default_xattr_handler into s_xattr and use the
> generic_{get,set,remove}xattr inode operations to hook things up
> appropriately. The xattr handlers convert to/from xattrs and struct posix_acl
> and call the get_acl and set_acl inode operations. That's where the namespace
> conversion is supposed to happen as well; this avoids special-casing
> it in the VFS.

The acl support does look like what I would expect this code to look
like.  Unfortunately some fileystems still (as of 4.6) write xattrs
directly to disk.

> The only filesystems that don't do things that way are CIFS and Lustre. Doing
> the appropriate namespace mapping in CIFS is easy (see this patch). So it's only
> Lustre that's left with the original in-place xattr conversion.

You know I really wish this was the case.  And perhaps I need to look to
see what has just been merged in the merge window.  There are clearly
some xattr changes post 4.6 that I am not seeing.

As of 4.6.0 writing xattrs from a user namespace is broken by this
change.

> There have been cleanups in this area before, so I assume things were
> fixed. In any case, I didn't blindly hack this together, I've actually checked
> all the filesystems.

You know that scares me.  Because I just started looking and the first
oddball case I checked was broken.
>
>> There is a complication that is comming shortly (next merge window
>> unless major unfixable bugs show up between now and them).  There is a
>> new field s_user_ns that is being introduced for filesystems to record
>> their owner and their default translation rules to kuids.  That will
>> make the hard coded &init_user_ns values in your patch wrong.
>
> Okay, that conflict should be easy enough to resolve. Where's that code?
>
>>>> Please Cc the user namespace maintainers before. Thank you!
>>>
>>> Eric, Andy, anyone else?
>>
>> Serge Hallyn has a pending patch that adds a similar translation to
>> the security.capability xattr.  Which is one more case of where caching
>> and translation at the VFS layer are makes sense.
>
> No. Just like with this patch, that mapping really needs to go into the
> appropriate xattr handler. Again, where's that code, please?

I will start looking at what is in Linus's tree.  But unless it happens
to address my concerns, I am not even going to consider the notion that
things should be in an ``xattr handler''.

There are very good reasons why that conversion does not, nor should not
happen inside of filesystem specific code.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman May 27, 2016, 6:26 p.m. UTC | #14
ebiederm@xmission.com (Eric W. Biederman) writes:
>>
>> No. Just like with this patch, that mapping really needs to go into the
>> appropriate xattr handler. Again, where's that code, please?
>
> I will start looking at what is in Linus's tree.  But unless it happens
> to address my concerns, I am not even going to consider the notion that
> things should be in an ``xattr handler''.
>
> There are very good reasons why that conversion does not, nor should not
> happen inside of filesystem specific code.

Ok.  I have looked.

The cleanups in Linus' tree seem worthwhile.  The notion that mapping
needs to go into a non-generic xattr handler is broken.

If everything can always use a single set of acl handlers for posix acls
then I will be happy to consider something like your patch.

Fundamentally for vfs and security module supported attributes what a user
writes in setxattr needs to be disconnected from what actually gets
written to disk.

Things evolve and userspace breaks if we don't handle the compatibility
between old and new in the kernel.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/staging/lustre/lustre/llite/Makefile b/drivers/staging/lustre/lustre/llite/Makefile
index 2ce10ff..67125f8 100644
--- a/drivers/staging/lustre/lustre/llite/Makefile
+++ b/drivers/staging/lustre/lustre/llite/Makefile
@@ -7,5 +7,6 @@  lustre-y := dcache.o dir.o file.o llite_close.o llite_lib.o llite_nfs.o \
 	    glimpse.o lcommon_cl.o lcommon_misc.o \
 	    vvp_dev.o vvp_page.o vvp_lock.o vvp_io.o vvp_object.o vvp_req.o \
 	    lproc_llite.o
+lustre-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
 
 llite_lloop-y := lloop.o
diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
index ce1f949..d454dfb 100644
--- a/drivers/staging/lustre/lustre/llite/llite_internal.h
+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
@@ -1402,4 +1402,7 @@  int cl_local_size(struct inode *inode);
 __u64 cl_fid_build_ino(const struct lu_fid *fid, int api32);
 __u32 cl_fid_build_gen(const struct lu_fid *fid);
 
+void posix_acl_fix_xattr_from_user(void *value, size_t size);
+void posix_acl_fix_xattr_to_user(void *value, size_t size);
+
 #endif /* LLITE_INTERNAL_H */
diff --git a/drivers/staging/lustre/lustre/llite/posix_acl.c b/drivers/staging/lustre/lustre/llite/posix_acl.c
new file mode 100644
index 0000000..4fabd0f
--- /dev/null
+++ b/drivers/staging/lustre/lustre/llite/posix_acl.c
@@ -0,0 +1,62 @@ 
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/posix_acl_xattr.h>
+#include <linux/user_namespace.h>
+
+/*
+ * Fix up the uids and gids in posix acl extended attributes in place.
+ */
+static void posix_acl_fix_xattr_userns(
+	struct user_namespace *to, struct user_namespace *from,
+	void *value, size_t size)
+{
+	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
+	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
+	int count;
+	kuid_t uid;
+	kgid_t gid;
+
+	if (!value)
+		return;
+	if (size < sizeof(posix_acl_xattr_header))
+		return;
+	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
+		return;
+
+	count = posix_acl_xattr_count(size);
+	if (count < 0)
+		return;
+	if (count == 0)
+		return;
+
+	for (end = entry + count; entry != end; entry++) {
+		switch(le16_to_cpu(entry->e_tag)) {
+		case ACL_USER:
+			uid = make_kuid(from, le32_to_cpu(entry->e_id));
+			entry->e_id = cpu_to_le32(from_kuid(to, uid));
+			break;
+		case ACL_GROUP:
+			gid = make_kgid(from, le32_to_cpu(entry->e_id));
+			entry->e_id = cpu_to_le32(from_kgid(to, gid));
+			break;
+		default:
+			break;
+		}
+	}
+}
+
+void posix_acl_fix_xattr_from_user(void *value, size_t size)
+{
+	struct user_namespace *user_ns = current_user_ns();
+	if (user_ns == &init_user_ns)
+		return;
+	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
+}
+
+void posix_acl_fix_xattr_to_user(void *value, size_t size)
+{
+	struct user_namespace *user_ns = current_user_ns();
+	if (user_ns == &init_user_ns)
+		return;
+	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
+}
diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
index ed4de04..c721b44 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -144,6 +144,9 @@  int ll_setxattr_common(struct inode *inode, const char *name,
 		return -EOPNOTSUPP;
 
 #ifdef CONFIG_FS_POSIX_ACL
+	if (xattr_type == XATTR_ACL_ACCESS_T ||
+	    xattr_type == XATTR_ACL_DEFAULT_T)
+		posix_acl_fix_xattr_from_user((void *)value, size);
 	if (sbi->ll_flags & LL_SBI_RMT_CLIENT &&
 	    (xattr_type == XATTR_ACL_ACCESS_T ||
 	    xattr_type == XATTR_ACL_DEFAULT_T)) {
@@ -348,7 +351,7 @@  int ll_getxattr_common(struct inode *inode, const char *name,
 		if (!acl)
 			return -ENODATA;
 
-		rc = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
+		rc = posix_acl_to_xattr(current_user_ns(), acl, buffer, size);
 		posix_acl_release(acl);
 		return rc;
 	}
@@ -436,6 +439,9 @@  getxattr_nocache:
 			goto out;
 		}
 	}
+	if (rc >= 0 && (xattr_type == XATTR_ACL_ACCESS_T ||
+			xattr_type == XATTR_ACL_DEFAULT_T))
+		posix_acl_fix_xattr_to_user(buffer, rc);
 #endif
 
 out_xattr:
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index d47197e..9dc001f 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -3337,10 +3337,25 @@  CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
 static void cifs_convert_ace(posix_acl_xattr_entry *ace,
 			     struct cifs_posix_ace *cifs_ace)
 {
+	u32 cifs_id, id = -1;
+
 	/* u8 cifs fields do not need le conversion */
 	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
 	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
-	ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
+	switch(cifs_ace->cifs_e_tag) {
+	case ACL_USER:
+		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
+		id = from_kuid(current_user_ns(),
+			       make_kuid(&init_user_ns, cifs_id));
+		break;
+
+	case ACL_GROUP:
+		cifs_id = le64_to_cpu(cifs_ace->cifs_uid);
+		id = from_kgid(current_user_ns(),
+			       make_kgid(&init_user_ns, cifs_id));
+		break;
+	}
+	ace->e_id = cpu_to_le32(id);
 /*
 	cifs_dbg(FYI, "perm %d tag %d id %d\n",
 		 ace->e_perm, ace->e_tag, ace->e_id);
@@ -3408,21 +3423,29 @@  static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
 static __u16 convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
 				     const posix_acl_xattr_entry *local_ace)
 {
-	__u16 rc = 0; /* 0 = ACL converted ok */
+	u32 cifs_id = -1, id;
 
 	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
 	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
-	/* BB is there a better way to handle the large uid? */
-	if (local_ace->e_id == cpu_to_le32(-1)) {
-	/* Probably no need to le convert -1 on any arch but can not hurt */
-		cifs_ace->cifs_uid = cpu_to_le64(-1);
-	} else
-		cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
+	switch(cifs_ace->cifs_e_tag) {
+	case ACL_USER:
+		id = le32_to_cpu(local_ace->e_id);
+		cifs_id = from_kuid(&init_user_ns,
+				    make_kuid(current_user_ns(), id));
+		break;
+
+	case ACL_GROUP:
+		id = le32_to_cpu(local_ace->e_id);
+		cifs_id = from_kgid(&init_user_ns,
+				    make_kgid(current_user_ns(), id));
+		break;
+	}
+	cifs_ace->cifs_uid = cpu_to_le64(cifs_id);
 /*
 	cifs_dbg(FYI, "perm %d tag %d id %d\n",
 		 ace->e_perm, ace->e_tag, ace->e_id);
 */
-	return rc;
+	return 0;
 }
 
 /* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 2c60f17..fca281c 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -627,64 +627,6 @@  no_mem:
 EXPORT_SYMBOL_GPL(posix_acl_create);
 
 /*
- * Fix up the uids and gids in posix acl extended attributes in place.
- */
-static void posix_acl_fix_xattr_userns(
-	struct user_namespace *to, struct user_namespace *from,
-	void *value, size_t size)
-{
-	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
-	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
-	int count;
-	kuid_t uid;
-	kgid_t gid;
-
-	if (!value)
-		return;
-	if (size < sizeof(posix_acl_xattr_header))
-		return;
-	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
-		return;
-
-	count = posix_acl_xattr_count(size);
-	if (count < 0)
-		return;
-	if (count == 0)
-		return;
-
-	for (end = entry + count; entry != end; entry++) {
-		switch(le16_to_cpu(entry->e_tag)) {
-		case ACL_USER:
-			uid = make_kuid(from, le32_to_cpu(entry->e_id));
-			entry->e_id = cpu_to_le32(from_kuid(to, uid));
-			break;
-		case ACL_GROUP:
-			gid = make_kgid(from, le32_to_cpu(entry->e_id));
-			entry->e_id = cpu_to_le32(from_kgid(to, gid));
-			break;
-		default:
-			break;
-		}
-	}
-}
-
-void posix_acl_fix_xattr_from_user(void *value, size_t size)
-{
-	struct user_namespace *user_ns = current_user_ns();
-	if (user_ns == &init_user_ns)
-		return;
-	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
-}
-
-void posix_acl_fix_xattr_to_user(void *value, size_t size)
-{
-	struct user_namespace *user_ns = current_user_ns();
-	if (user_ns == &init_user_ns)
-		return;
-	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
-}
-
-/*
  * Convert from extended attribute to in-memory representation.
  */
 struct posix_acl *
@@ -814,7 +756,7 @@  posix_acl_xattr_get(const struct xattr_handler *handler,
 	if (acl == NULL)
 		return -ENODATA;
 
-	error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
+	error = posix_acl_to_xattr(current_user_ns(), acl, value, size);
 	posix_acl_release(acl);
 
 	return error;
@@ -840,7 +782,7 @@  posix_acl_xattr_set(const struct xattr_handler *handler,
 		return -EPERM;
 
 	if (value) {
-		acl = posix_acl_from_xattr(&init_user_ns, value, size);
+		acl = posix_acl_from_xattr(current_user_ns(), value, size);
 		if (IS_ERR(acl))
 			return PTR_ERR(acl);
 
diff --git a/fs/xattr.c b/fs/xattr.c
index b11945e..b648b05 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -20,7 +20,6 @@ 
 #include <linux/fsnotify.h>
 #include <linux/audit.h>
 #include <linux/vmalloc.h>
-#include <linux/posix_acl_xattr.h>
 
 #include <asm/uaccess.h>
 
@@ -329,9 +328,6 @@  setxattr(struct dentry *d, const char __user *name, const void __user *value,
 			error = -EFAULT;
 			goto out;
 		}
-		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
-		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
-			posix_acl_fix_xattr_from_user(kvalue, size);
 	}
 
 	error = vfs_setxattr(d, kname, kvalue, size, flags);
@@ -426,9 +422,6 @@  getxattr(struct dentry *d, const char __user *name, void __user *value,
 
 	error = vfs_getxattr(d, kname, kvalue, size);
 	if (error > 0) {
-		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
-		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
-			posix_acl_fix_xattr_to_user(kvalue, size);
 		if (size && copy_to_user(value, kvalue, error))
 			error = -EFAULT;
 	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
index e5e8ec4..9789aba 100644
--- a/include/linux/posix_acl_xattr.h
+++ b/include/linux/posix_acl_xattr.h
@@ -48,18 +48,6 @@  posix_acl_xattr_count(size_t size)
 	return size / sizeof(posix_acl_xattr_entry);
 }
 
-#ifdef CONFIG_FS_POSIX_ACL
-void posix_acl_fix_xattr_from_user(void *value, size_t size);
-void posix_acl_fix_xattr_to_user(void *value, size_t size);
-#else
-static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
-{
-}
-static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
-{
-}
-#endif
-
 struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns, 
 				       const void *value, size_t size);
 int posix_acl_to_xattr(struct user_namespace *user_ns,