Re: [Patch] Support overriding uid/gid in overlayfs

Re: [Patch] Support overriding uid/gid in overlayfs

[Miklos Szeredi]

On Tue, Jan 15, 2013 at 1:58 PM, Alessandro Pignotti
<alexpigna.dev@gmail.com> wrote:
> Hi Miklos,
>
> I hope you are the right person to send this. I've been working over the
> last few days to add uid/gid overriding support in overlayfs. I think
> the patch is in a fairly good state, but I'm open to suggestions about
> how to make it good enough for acceptance in your tree.

Please add linux-fsdevel list for overlayfs related posts.

>
> The main idea is that the uid/gid of overlayfs inodes are overridden if
> needed so that the user space sees only the custom uid/gids.

Why?

>
> Currently, even when a custom uid or gid is used, files in the upperdir
> are created with the actual fsuid/fsgid of the current user (as usual),
> but they are still seen as being owned by the custom uid/gid. I'm not
> sure if this is the best approach, another possibility would be to
> impersonate the custom uid/gid when creating objects in upperdir. I'm
> open to suggestions about the best way to handle this situation.

I cannot say without knowing more about what this will be used for.

Thanks,
Miklos

Re: [Patch] Support overriding uid/gid in overlayfs

[Alessandro Pignotti]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

Hi everyone,

the proposed patch would be useful in the following scenario

1) There is a large directory tree which needs to be fully read/write
accessible from many different users, although in an isolated way (i.d.
the changes made by Alice should not be visible to Bob)
2) All users expect to be owning the files. (i.d. that everything is
private)

With the proposed patch it is possible to store a single copy of the
base directory tree and use it as the lowerdir. The upperdir is then set
to a private user-writable directory. Each user as the illusion of a
private fully-writable installation as required. Moreover, since all the
changed files ends up in the upperdir it's easy to pack them for backup
purposes.

I'm attaching an improved version of the patch, which is being currently
used in our setup and seems to be working ok. I think this functionality
may be useful to others and I'd like to receive feedback on how to
improve it to make it fit for upstream inclusion.

Regards,
Alessandro Pignotti

Il giorno ven, 25/01/2013 alle 12.08 +0100, Miklos Szeredi ha scritto:
> On Tue, Jan 15, 2013 at 1:58 PM, Alessandro Pignotti
> <alexpigna.dev@gmail.com> wrote:
> > Hi Miklos,
> >
> > I hope you are the right person to send this. I've been working over the
> > last few days to add uid/gid overriding support in overlayfs. I think
> > the patch is in a fairly good state, but I'm open to suggestions about
> > how to make it good enough for acceptance in your tree.
> 
> Please add linux-fsdevel list for overlayfs related posts.
> 
> >
> > The main idea is that the uid/gid of overlayfs inodes are overridden if
> > needed so that the user space sees only the custom uid/gids.
> 
> Why?
> 
> >
> > Currently, even when a custom uid or gid is used, files in the upperdir
> > are created with the actual fsuid/fsgid of the current user (as usual),
> > but they are still seen as being owned by the custom uid/gid. I'm not
> > sure if this is the best approach, another possibility would be to
> > impersonate the custom uid/gid when creating objects in upperdir. I'm
> > open to suggestions about the best way to handle this situation.
> 
> I cannot say without knowing more about what this will be used for.
> 
> Thanks,
> Miklos


[-- Attachment #2: 0001-overlayfs-uid-gid-v3.patch --]
[-- Type: text/x-patch, Size: 8074 bytes --]

diff -ru linux-3.5.0/fs/overlayfs/copy_up.c linux-3.5.0.mod/fs/overlayfs/copy_up.c
--- linux-3.5.0/fs/overlayfs/copy_up.c	2013-01-10 17:28:48.000000000 +0100
+++ linux-3.5.0.mod/fs/overlayfs/copy_up.c	2013-01-15 13:34:00.786821595 +0100
@@ -320,6 +320,7 @@
 int ovl_copy_up(struct dentry *dentry)
 {
 	int err;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
 	err = 0;
 	while (!err) {
@@ -347,6 +348,7 @@
 
 		ovl_path_lower(next, &lowerpath);
 		err = vfs_getattr(lowerpath.mnt, lowerpath.dentry, &stat);
+		ovl_filter_stat(&ofs->config, &stat);
 		if (!err)
 			err = ovl_copy_up_one(parent, next, &lowerpath, &stat);
 
diff -ru linux-3.5.0/fs/overlayfs/dir.c linux-3.5.0.mod/fs/overlayfs/dir.c
--- linux-3.5.0/fs/overlayfs/dir.c	2013-01-10 17:28:48.000000000 +0100
+++ linux-3.5.0.mod/fs/overlayfs/dir.c	2013-01-11 16:40:06.708419408 +0100
@@ -245,6 +245,7 @@
 	int err;
 	enum ovl_path_type type;
 	struct path realpath;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
 	type = ovl_path_real(dentry, &realpath);
 	err = vfs_getattr(realpath.mnt, realpath.dentry, stat);
@@ -253,6 +254,7 @@
 
 	stat->dev = dentry->d_sb->s_dev;
 	stat->ino = dentry->d_inode->i_ino;
+	ovl_filter_stat(&ofs->config, stat);
 
 	/*
 	 * It's probably not worth it to count subdirs to get the
@@ -269,6 +271,7 @@
 			     const char *link)
 {
 	int err;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct dentry *newdentry;
 	struct dentry *upperdir;
 	struct inode *inode;
@@ -304,7 +307,7 @@
 		}
 	}
 	ovl_dentry_update(dentry, newdentry);
-	ovl_copyattr(newdentry->d_inode, inode);
+	ovl_copyattr(&ofs->config,newdentry->d_inode, inode);
 	d_instantiate(dentry, inode);
 	inode = NULL;
 	newdentry = NULL;
@@ -415,6 +418,7 @@
 		    struct dentry *new)
 {
 	int err;
+	struct ovl_fs *ofs = old->d_sb->s_fs_info;
 	struct dentry *olddentry;
 	struct dentry *newdentry;
 	struct dentry *upperdir;
@@ -447,7 +451,7 @@
 				new->d_fsdata);
 		if (!newinode)
 			goto link_fail;
-		ovl_copyattr(upperdir->d_inode, newinode);
+		ovl_copyattr(&ofs->config,upperdir->d_inode, newinode);
 
 		ovl_dentry_version_inc(new->d_parent);
 		ovl_dentry_update(new, newdentry);
diff -ru linux-3.5.0/fs/overlayfs/inode.c linux-3.5.0.mod/fs/overlayfs/inode.c
--- linux-3.5.0/fs/overlayfs/inode.c	2013-01-10 17:28:48.000000000 +0100
+++ linux-3.5.0.mod/fs/overlayfs/inode.c	2013-01-14 16:57:50.984199297 +0100
@@ -10,6 +10,7 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/xattr.h>
+#include <linux/sched.h>
 #include "overlayfs.h"
 
 int ovl_setattr(struct dentry *dentry, struct iattr *attr)
@@ -40,9 +41,15 @@
 			 struct kstat *stat)
 {
 	struct path realpath;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+	int err;
 
 	ovl_path_real(dentry, &realpath);
-	return vfs_getattr(realpath.mnt, realpath.dentry, stat);
+	err = vfs_getattr(realpath.mnt, realpath.dentry, stat);
+	if(err)
+		return err;
+	ovl_filter_stat(&ofs->config, stat);
+	return 0;
 }
 
 int ovl_permission(struct inode *inode, int mask)
@@ -51,6 +58,9 @@
 	struct dentry *alias = NULL;
 	struct inode *realinode;
 	struct dentry *realdentry;
+	struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+	const struct cred *old_cred = NULL;
+	struct cred *override_cred;
 	bool is_upper;
 	int err;
 
@@ -107,7 +117,38 @@
 			goto out_dput;
 	}
 
+	/* If
+	 * 1) We have custom uid or gid set
+	 * 2) The current uid or gid is the same as those
+	 * 3) The current uid and git are _not_ already equal to the file ones
+	 * then we have to pretend being the actual owner of the file
+	 * */
+	int should_change_fs_uid = ofs->config.use_uid &&
+		current_fsuid() == ofs->config.uid &&
+		current_fsuid() != realinode->i_uid;
+	int should_change_fs_gid = ofs->config.use_gid &&
+		current_fsgid() == ofs->config.gid &&
+		current_fsgid() != realinode->i_gid;
+	if(should_change_fs_uid || should_change_fs_gid)
+	{
+		if (mask & MAY_NOT_BLOCK)
+			return -ECHILD;
+
+		override_cred = prepare_creds();
+		if (!override_cred)
+			goto out_dput;
+		if(should_change_fs_uid)
+			override_cred->fsuid = realinode->i_uid;
+		if(should_change_fs_gid)
+			override_cred->fsgid = realinode->i_gid;
+		old_cred = override_creds(override_cred);
+	}
 	err = inode_only_permission(realinode, mask);
+	if(old_cred)
+	{
+		revert_creds(old_cred);
+		put_cred(override_cred);
+	}
 out_dput:
 	dput(alias);
 	return err;
diff -ru linux-3.5.0/fs/overlayfs/overlayfs.h linux-3.5.0.mod/fs/overlayfs/overlayfs.h
--- linux-3.5.0/fs/overlayfs/overlayfs.h	2013-01-10 17:28:48.000000000 +0100
+++ linux-3.5.0.mod/fs/overlayfs/overlayfs.h	2013-01-15 11:46:28.573232080 +0100
@@ -9,6 +9,23 @@
 
 struct ovl_entry;
 
+struct ovl_config {
+	char *lowerdir;
+	char *upperdir;
+	kuid_t uid;
+	kgid_t gid;
+	int use_uid:1;
+	int use_gid:1;
+};
+
+/* private information held for overlayfs's superblock */
+struct ovl_fs {
+	struct vfsmount *upper_mnt;
+	struct vfsmount *lower_mnt;
+	/* pathnames of lower and upper dirs, for show_options */
+	struct ovl_config config;
+};
+
 enum ovl_path_type {
 	OVL_PATH_UPPER,
 	OVL_PATH_MERGE,
@@ -56,10 +73,19 @@
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode,
 			    struct ovl_entry *oe);
-static inline void ovl_copyattr(struct inode *from, struct inode *to)
+static inline void ovl_copyattr(const struct ovl_config* config, struct inode *from, struct inode *to)
+{
+	to->i_uid = (config->use_uid)?config->uid:from->i_uid;
+	to->i_gid = (config->use_gid)?config->gid:from->i_gid;
+}
+
+static inline void ovl_filter_stat(const struct ovl_config* config, struct kstat* stat)
 {
-	to->i_uid = from->i_uid;
-	to->i_gid = from->i_gid;
+	/* Override the uid and gid with the ones provided if needed */
+	if(config->use_uid)
+		stat->uid = config->uid;
+	if(config->use_gid)
+		stat->gid = config->gid;
 }
 
 /* dir.c */
diff -ru linux-3.5.0/fs/overlayfs/super.c linux-3.5.0.mod/fs/overlayfs/super.c
--- linux-3.5.0/fs/overlayfs/super.c	2013-01-10 17:28:48.000000000 +0100
+++ linux-3.5.0.mod/fs/overlayfs/super.c	2013-01-10 13:32:52.563327259 +0100
@@ -24,19 +24,6 @@
 MODULE_DESCRIPTION("Overlay filesystem");
 MODULE_LICENSE("GPL");
 
-struct ovl_config {
-	char *lowerdir;
-	char *upperdir;
-};
-
-/* private information held for overlayfs's superblock */
-struct ovl_fs {
-	struct vfsmount *upper_mnt;
-	struct vfsmount *lower_mnt;
-	/* pathnames of lower and upper dirs, for show_options */
-	struct ovl_config config;
-};
-
 /* private information held for every overlayfs dentry */
 struct ovl_entry {
 	/*
@@ -272,6 +259,7 @@
 static int ovl_do_lookup(struct dentry *dentry)
 {
 	struct ovl_entry *oe;
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct dentry *upperdir;
 	struct dentry *lowerdir;
 	struct dentry *upperdentry = NULL;
@@ -343,7 +331,7 @@
 				      oe);
 		if (!inode)
 			goto out_dput;
-		ovl_copyattr(realdentry->d_inode, inode);
+		ovl_copyattr(&ofs->config,realdentry->d_inode, inode);
 	}
 
 	if (upperdentry)
@@ -464,21 +452,30 @@
 enum {
 	Opt_lowerdir,
 	Opt_upperdir,
+	Opt_uid,
+	Opt_gid,
 	Opt_err,
 };
 
 static const match_table_t ovl_tokens = {
 	{Opt_lowerdir,			"lowerdir=%s"},
 	{Opt_upperdir,			"upperdir=%s"},
+	{Opt_uid,			"uid=%d"},
+	{Opt_gid,			"gid=%d"},
 	{Opt_err,			NULL}
 };
 
 static int ovl_parse_opt(char *opt, struct ovl_config *config)
 {
 	char *p;
+	int option;
 
 	config->upperdir = NULL;
 	config->lowerdir = NULL;
+	config->use_uid = 0;
+	config->use_gid = 0;
+	config->uid = 0;
+	config->gid = 0;
 
 	while ((p = strsep(&opt, ",")) != NULL) {
 		int token;
@@ -503,6 +500,25 @@
 				return -ENOMEM;
 			break;
 
+		case Opt_uid:
+			if(match_int(&args[0], &option))
+				return -EINVAL;
+			config->uid = make_kuid(current_user_ns(), option);
+			if(!uid_valid(config->uid))
+				return -EINVAL;
+			config->use_uid = 1;
+			printk("New uid\n");
+			break;
+
+		case Opt_gid:
+			if(match_int(&args[0], &option))
+				return -EINVAL;
+			config->gid = make_kgid(current_user_ns(), option);
+			if(!gid_valid(config->gid))
+				return -EINVAL;
+			config->use_gid = 1;
+			break;
+
 		default:
 			return -EINVAL;
 		}

Re: [Patch] Support overriding uid/gid in overlayfs

[Miklos Szeredi]

On Mon, Jan 28, 2013 at 4:58 PM, Alessandro Pignotti
<alexpigna.dev@gmail.com> wrote:
> Hi everyone,
>
> the proposed patch would be useful in the following scenario
>
> 1) There is a large directory tree which needs to be fully read/write
> accessible from many different users, although in an isolated way (i.d.
> the changes made by Alice should not be visible to Bob)
> 2) All users expect to be owning the files. (i.d. that everything is
> private)
>
> With the proposed patch it is possible to store a single copy of the
> base directory tree and use it as the lowerdir. The upperdir is then set
> to a private user-writable directory. Each user as the illusion of a
> private fully-writable installation as required. Moreover, since all the
> changed files ends up in the upperdir it's easy to pack them for backup
> purposes.

Okay, makes sense.

One issue, though, is that stat("x") and fd=open("x"); fstat(fd) will
return different file stats (the fstat one will return the stats for
the original file).   This may confuse some apps.

This is a something a "true" union filesystem can do better that
union-mounts or overlayfs.

Thanks,
Miklos

Re: [Patch] Support overriding uid/gid in overlayfs

[J. R. Okajima]

Miklos Szeredi:
> One issue, though, is that stat("x") and fd=open("x"); fstat(fd) will
> return different file stats (the fstat one will return the stats for
> the original file).   This may confuse some apps.
>
> This is a something a "true" union filesystem can do better that
> union-mounts or overlayfs.

Exactly.
That was one example I picked up and posted when I read overlayfs years
ago.
Recently I named it "name-based union". Of course AUFS is "inode-based
union." stat(2) operates upon a filename while fstat(2) operates upon a
file descriptor. Overlayfs (and probably UnionMount too) may return
different info for these two systemcalls even when they should be equal.
All the inode-based (or file descriptor based) operations can confuse
users in overlayfs, I am afraid. I was looking forward to see how
overlayfs solve this problem.

Overriding uid/gid is an idea, but obviously it will not match using
with system dirs such as /bin and /usr, since they often have suid
binaries. So it will be used only for user dirs which is unioned as
lower RO layer. It might be better to implement such feature into your
lower FS (or generic VFS feature) instead of overlayfs.
If I remember correctly, some non-unix FS such as VFAT already have such
feature.


J. R. Okajima

Re: [Patch] Support overriding uid/gid in overlayfs

[Miklos Szeredi]

On Thu, Jan 31, 2013 at 6:01 PM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>
> Miklos Szeredi:
>> One issue, though, is that stat("x") and fd=open("x"); fstat(fd) will
>> return different file stats (the fstat one will return the stats for
>> the original file).   This may confuse some apps.
>>
>> This is a something a "true" union filesystem can do better that
>> union-mounts or overlayfs.
>
> Exactly.
> That was one example I picked up and posted when I read overlayfs years
> ago.
> Recently I named it "name-based union". Of course AUFS is "inode-based
> union." stat(2) operates upon a filename while fstat(2) operates upon a
> file descriptor. Overlayfs (and probably UnionMount too) may return
> different info for these two systemcalls even when they should be equal.
> All the inode-based (or file descriptor based) operations can confuse
> users in overlayfs, I am afraid. I was looking forward to see how
> overlayfs solve this problem.

They don't "solve" this, it's a fundamental property of the
implementation.  Overlayfs and union-mounts behave as if copy-up was a
"bind mount" over the file in question.  Yes, it's a namespace trick
but it seems to work in most situations.

>
> Overriding uid/gid is an idea, but obviously it will not match using
> with system dirs such as /bin and /usr, since they often have suid
> binaries. So it will be used only for user dirs which is unioned as
> lower RO layer. It might be better to implement such feature into your
> lower FS (or generic VFS feature) instead of overlayfs.
> If I remember correctly, some non-unix FS such as VFAT already have such
> feature.

The point was that uid/gid was to be overridden with *different*
values for each overlayfs instance.  So implementing uid/gid
overriding in the lower fs doesn't help.

Thanks,
Miklos

Re: [Patch] Support overriding uid/gid in overlayfs

[J. R. Okajima]

Miklos Szeredi:
> They don't "solve" this, it's a fundamental property of the
> implementation.  Overlayfs and union-mounts behave as if copy-up was a
> "bind mount" over the file in question.  Yes, it's a namespace trick
> but it seems to work in most situations.

It must be a sad news for overlayfs users who have every met "non-most"
situations.


> The point was that uid/gid was to be overridden with *different*
> values for each overlayfs instance.  So implementing uid/gid
> overriding in the lower fs doesn't help.

For the scenario which Alessandro Pignotti posted,
- everything on the lower fs are owned by a single user, by the
  overriding feature.
- everything on the upper fs are owned by the user natively.
Isn't it enough?


J. R. Okajima