* [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link()
@ 2022-08-25 13:05 Zhang Tianci
2022-08-25 13:25 ` Christian Brauner
2022-08-31 13:00 ` Miklos Szeredi
0 siblings, 2 replies; 8+ messages in thread
From: Zhang Tianci @ 2022-08-25 13:05 UTC (permalink / raw)
To: miklos
Cc: Zhang Tianci, linux-unionfs, linux-kernel, amir73il,
Jiachen Zhang, Christian Brauner
There is a wrong case of link() on overlay:
$ mkdir /lower /fuse /merge
$ mount -t fuse /fuse
$ mkdir /fuse/upper /fuse/work
$ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\
workdir=work
$ touch /merge/file
$ chown bin.bin /merge/file // the file's caller becomes "bin"
$ ln /merge/file /merge/lnkfile
Then we will get an error(EACCES) because fuse daemon checks the link()'s
caller is "bin", it denied this request.
In the changing history of ovl_link(), there are two key commits:
The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which
overrides the cred's fsuid/fsgid using the new inode. The new inode's
owner is initialized by inode_init_owner(), and inode->fsuid is
assigned to the current user. So the override fsuid becomes the
current user. We know link() is actually modifying the directory, so
the caller must have the MAY_WRITE permission on the directory. The
current caller may should have this permission. This is acceptable
to use the caller's fsuid.
The second is commit 51f7e52dc943 ("ovl: share inode for hard link")
which removed the inode creation in ovl_link(). This commit move
inode_init_owner() into ovl_create_object(), so the ovl_link() just
give the old inode to ovl_create_or_link(). Then the override fsuid
becomes the old inode's fsuid, neither the caller nor the overlay's
creator! So this is incorrect.
Fix this bug by using current fsuid/fsgid to do underlying fs's link().
Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T
Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com>
Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
fs/overlayfs/dir.c | 16 +++++++++++-----
fs/overlayfs/overlayfs.h | 2 ++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 6b03457f72bb..dd84e6fc5f6e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
err = -ENOMEM;
override_cred = prepare_creds();
if (override_cred) {
- override_cred->fsuid = inode->i_uid;
- override_cred->fsgid = inode->i_gid;
+ override_cred->fsuid = attr->fsuid;
+ override_cred->fsgid = attr->fsgid;
if (!attr->hardlink) {
err = security_dentry_create_files_as(dentry,
attr->mode, &dentry->d_name, old_cred,
@@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
attr.mode = inode->i_mode;
+ attr.fsuid = inode->i_uid;
+ attr.fsgid = inode->i_gid;
err = ovl_create_or_link(dentry, inode, &attr, false);
/* Did we end up using the preallocated inode? */
if (inode != d_inode(dentry))
@@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
{
int err;
struct inode *inode;
+ struct ovl_cattr attr;
err = ovl_want_write(old);
if (err)
@@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
inode = d_inode(old);
ihold(inode);
- err = ovl_create_or_link(new, inode,
- &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)},
- ovl_type_origin(old));
+ attr = (struct ovl_cattr) {
+ .hardlink = ovl_dentry_upper(old),
+ .fsuid = current_fsuid(),
+ .fsgid = current_fsgid(),
+ };
+ err = ovl_create_or_link(new, inode, &attr, ovl_type_origin(old));
if (err)
iput(inode);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 87759165d32b..85043123a103 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -655,6 +655,8 @@ struct ovl_cattr {
umode_t mode;
const char *link;
struct dentry *hardlink;
+ kuid_t fsuid;
+ kgid_t fsgid;
};
#define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
--
2.32.1 (Apple Git-133)
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-25 13:05 [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() Zhang Tianci @ 2022-08-25 13:25 ` Christian Brauner 2022-08-30 2:44 ` [External] " 天赐张 2022-08-31 13:00 ` Miklos Szeredi 1 sibling, 1 reply; 8+ messages in thread From: Christian Brauner @ 2022-08-25 13:25 UTC (permalink / raw) To: Zhang Tianci; +Cc: miklos, linux-unionfs, linux-kernel, amir73il, Jiachen Zhang On Thu, Aug 25, 2022 at 09:05:52PM +0800, Zhang Tianci wrote: > There is a wrong case of link() on overlay: > $ mkdir /lower /fuse /merge > $ mount -t fuse /fuse > $ mkdir /fuse/upper /fuse/work > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > workdir=work > $ touch /merge/file > $ chown bin.bin /merge/file // the file's caller becomes "bin" > $ ln /merge/file /merge/lnkfile > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > caller is "bin", it denied this request. > > In the changing history of ovl_link(), there are two key commits: > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > overrides the cred's fsuid/fsgid using the new inode. The new inode's > owner is initialized by inode_init_owner(), and inode->fsuid is > assigned to the current user. So the override fsuid becomes the > current user. We know link() is actually modifying the directory, so > the caller must have the MAY_WRITE permission on the directory. The > current caller may should have this permission. This is acceptable > to use the caller's fsuid. > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > which removed the inode creation in ovl_link(). This commit move > inode_init_owner() into ovl_create_object(), so the ovl_link() just > give the old inode to ovl_create_or_link(). Then the override fsuid > becomes the old inode's fsuid, neither the caller nor the overlay's > creator! So this is incorrect. > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- (Should probably also use a Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>, I think. But not that important.) Looks good to me but this really should be tested to survive xfstests, Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org> > fs/overlayfs/dir.c | 16 +++++++++++----- > fs/overlayfs/overlayfs.h | 2 ++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 6b03457f72bb..dd84e6fc5f6e 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > err = -ENOMEM; > override_cred = prepare_creds(); > if (override_cred) { > - override_cred->fsuid = inode->i_uid; > - override_cred->fsgid = inode->i_gid; > + override_cred->fsuid = attr->fsuid; > + override_cred->fsgid = attr->fsgid; > if (!attr->hardlink) { > err = security_dentry_create_files_as(dentry, > attr->mode, &dentry->d_name, old_cred, > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > attr.mode = inode->i_mode; > > + attr.fsuid = inode->i_uid; > + attr.fsgid = inode->i_gid; > err = ovl_create_or_link(dentry, inode, &attr, false); > /* Did we end up using the preallocated inode? */ > if (inode != d_inode(dentry)) > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > { > int err; > struct inode *inode; > + struct ovl_cattr attr; > > err = ovl_want_write(old); > if (err) > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > inode = d_inode(old); > ihold(inode); > > - err = ovl_create_or_link(new, inode, > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > - ovl_type_origin(old)); > + attr = (struct ovl_cattr) { > + .hardlink = ovl_dentry_upper(old), > + .fsuid = current_fsuid(), > + .fsgid = current_fsgid(), > + }; > + err = ovl_create_or_link(new, inode, &attr, ovl_type_origin(old)); > if (err) > iput(inode); > > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index 87759165d32b..85043123a103 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -655,6 +655,8 @@ struct ovl_cattr { > umode_t mode; > const char *link; > struct dentry *hardlink; > + kuid_t fsuid; > + kgid_t fsgid; > }; > > #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) }) > -- > 2.32.1 (Apple Git-133) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [External] Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-25 13:25 ` Christian Brauner @ 2022-08-30 2:44 ` 天赐张 0 siblings, 0 replies; 8+ messages in thread From: 天赐张 @ 2022-08-30 2:44 UTC (permalink / raw) To: Christian Brauner Cc: miklos, linux-unionfs, linux-kernel, amir73il, Jiachen Zhang Christian Brauner <brauner@kernel.org> 于2022年8月25日周四 21:25写道： > > On Thu, Aug 25, 2022 at 09:05:52PM +0800, Zhang Tianci wrote: > > There is a wrong case of link() on overlay: > > $ mkdir /lower /fuse /merge > > $ mount -t fuse /fuse > > $ mkdir /fuse/upper /fuse/work > > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > > workdir=work > > $ touch /merge/file > > $ chown bin.bin /merge/file // the file's caller becomes "bin" > > $ ln /merge/file /merge/lnkfile > > > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > > caller is "bin", it denied this request. > > > > In the changing history of ovl_link(), there are two key commits: > > > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > > overrides the cred's fsuid/fsgid using the new inode. The new inode's > > owner is initialized by inode_init_owner(), and inode->fsuid is > > assigned to the current user. So the override fsuid becomes the > > current user. We know link() is actually modifying the directory, so > > the caller must have the MAY_WRITE permission on the directory. The > > current caller may should have this permission. This is acceptable > > to use the caller's fsuid. > > > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > > which removed the inode creation in ovl_link(). This commit move > > inode_init_owner() into ovl_create_object(), so the ovl_link() just > > give the old inode to ovl_create_or_link(). Then the override fsuid > > becomes the old inode's fsuid, neither the caller nor the overlay's > > creator! So this is incorrect. > > > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > --- > > (Should probably also use a > Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>, I > think. But not that important.) > > Looks good to me but this really should be tested to survive xfstests, > Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > > fs/overlayfs/dir.c | 16 +++++++++++----- > > fs/overlayfs/overlayfs.h | 2 ++ > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > index 6b03457f72bb..dd84e6fc5f6e 100644 > > --- a/fs/overlayfs/dir.c > > +++ b/fs/overlayfs/dir.c > > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > > err = -ENOMEM; > > override_cred = prepare_creds(); > > if (override_cred) { > > - override_cred->fsuid = inode->i_uid; > > - override_cred->fsgid = inode->i_gid; > > + override_cred->fsuid = attr->fsuid; > > + override_cred->fsgid = attr->fsgid; > > if (!attr->hardlink) { > > err = security_dentry_create_files_as(dentry, > > attr->mode, &dentry->d_name, old_cred, > > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > > attr.mode = inode->i_mode; > > > > + attr.fsuid = inode->i_uid; > > + attr.fsgid = inode->i_gid; > > err = ovl_create_or_link(dentry, inode, &attr, false); > > /* Did we end up using the preallocated inode? */ > > if (inode != d_inode(dentry)) > > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > { > > int err; > > struct inode *inode; > > + struct ovl_cattr attr; > > > > err = ovl_want_write(old); > > if (err) > > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > inode = d_inode(old); > > ihold(inode); > > > > - err = ovl_create_or_link(new, inode, > > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > > - ovl_type_origin(old)); > > + attr = (struct ovl_cattr) { > > + .hardlink = ovl_dentry_upper(old), > > + .fsuid = current_fsuid(), > > + .fsgid = current_fsgid(), > > + }; > > + err = ovl_create_or_link(new, inode, &attr, ovl_type_origin(old)); > > if (err) > > iput(inode); > > > > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > > index 87759165d32b..85043123a103 100644 > > --- a/fs/overlayfs/overlayfs.h > > +++ b/fs/overlayfs/overlayfs.h > > @@ -655,6 +655,8 @@ struct ovl_cattr { > > umode_t mode; > > const char *link; > > struct dentry *hardlink; > > + kuid_t fsuid; > > + kgid_t fsgid; > > }; > > > > #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) }) > > -- > > 2.32.1 (Apple Git-133) > > I tested this patch base on linux-6.0.0-rc3, and all were successful. I tested xfstest using this local config: export TEST_DEV=/dev/vdb1 export TEST_DIR=/mnt/test export SCRATCH_DEV=/dev/vdb2 export SCRATCH_MNT=/mnt/scratch FSTYP=xfs run ./check -overlay in xfstest. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-25 13:05 [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() Zhang Tianci 2022-08-25 13:25 ` Christian Brauner @ 2022-08-31 13:00 ` Miklos Szeredi 2022-08-31 13:43 ` Christian Brauner 1 sibling, 1 reply; 8+ messages in thread From: Miklos Szeredi @ 2022-08-31 13:00 UTC (permalink / raw) To: Zhang Tianci Cc: linux-unionfs, linux-kernel, amir73il, Jiachen Zhang, Christian Brauner On Thu, 25 Aug 2022 at 15:08, Zhang Tianci <zhangtianci.1997@bytedance.com> wrote: > > There is a wrong case of link() on overlay: > $ mkdir /lower /fuse /merge > $ mount -t fuse /fuse > $ mkdir /fuse/upper /fuse/work > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > workdir=work > $ touch /merge/file > $ chown bin.bin /merge/file // the file's caller becomes "bin" > $ ln /merge/file /merge/lnkfile > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > caller is "bin", it denied this request. > > In the changing history of ovl_link(), there are two key commits: > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > overrides the cred's fsuid/fsgid using the new inode. The new inode's > owner is initialized by inode_init_owner(), and inode->fsuid is > assigned to the current user. So the override fsuid becomes the > current user. We know link() is actually modifying the directory, so > the caller must have the MAY_WRITE permission on the directory. The > current caller may should have this permission. This is acceptable > to use the caller's fsuid. > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > which removed the inode creation in ovl_link(). This commit move > inode_init_owner() into ovl_create_object(), so the ovl_link() just > give the old inode to ovl_create_or_link(). Then the override fsuid > becomes the old inode's fsuid, neither the caller nor the overlay's > creator! So this is incorrect. > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > fs/overlayfs/dir.c | 16 +++++++++++----- > fs/overlayfs/overlayfs.h | 2 ++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 6b03457f72bb..dd84e6fc5f6e 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > err = -ENOMEM; > override_cred = prepare_creds(); > if (override_cred) { > - override_cred->fsuid = inode->i_uid; > - override_cred->fsgid = inode->i_gid; > + override_cred->fsuid = attr->fsuid; > + override_cred->fsgid = attr->fsgid; > if (!attr->hardlink) { > err = security_dentry_create_files_as(dentry, > attr->mode, &dentry->d_name, old_cred, > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > attr.mode = inode->i_mode; > > + attr.fsuid = inode->i_uid; > + attr.fsgid = inode->i_gid; > err = ovl_create_or_link(dentry, inode, &attr, false); > /* Did we end up using the preallocated inode? */ > if (inode != d_inode(dentry)) > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > { > int err; > struct inode *inode; > + struct ovl_cattr attr; > > err = ovl_want_write(old); > if (err) > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > inode = d_inode(old); > ihold(inode); > > - err = ovl_create_or_link(new, inode, > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > - ovl_type_origin(old)); > + attr = (struct ovl_cattr) { > + .hardlink = ovl_dentry_upper(old), > + .fsuid = current_fsuid(), > + .fsgid = current_fsgid(), > + }; Why do we need to override fsuid/fsgid for the hardlink case? Wouldn't it be simpler to just use the mounter's creds unmodified in this case? The inode is not created in this case, so overriding with current uid/gid is not necessary, I think. Another way to look at it is: rename(A, B) is equivalent to an imaginary atomic [link(A, B) + unlink(A)] pair. But we don't override uid/gid for rename() or unlink() so why should we for link(). Thanks, Miklos ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-31 13:00 ` Miklos Szeredi @ 2022-08-31 13:43 ` Christian Brauner 2022-08-31 13:53 ` Miklos Szeredi 0 siblings, 1 reply; 8+ messages in thread From: Christian Brauner @ 2022-08-31 13:43 UTC (permalink / raw) To: Miklos Szeredi Cc: Zhang Tianci, linux-unionfs, linux-kernel, amir73il, Jiachen Zhang On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote: > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci > <zhangtianci.1997@bytedance.com> wrote: > > > > There is a wrong case of link() on overlay: > > $ mkdir /lower /fuse /merge > > $ mount -t fuse /fuse > > $ mkdir /fuse/upper /fuse/work > > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > > workdir=work > > $ touch /merge/file > > $ chown bin.bin /merge/file // the file's caller becomes "bin" > > $ ln /merge/file /merge/lnkfile > > > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > > caller is "bin", it denied this request. > > > > In the changing history of ovl_link(), there are two key commits: > > > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > > overrides the cred's fsuid/fsgid using the new inode. The new inode's > > owner is initialized by inode_init_owner(), and inode->fsuid is > > assigned to the current user. So the override fsuid becomes the > > current user. We know link() is actually modifying the directory, so > > the caller must have the MAY_WRITE permission on the directory. The > > current caller may should have this permission. This is acceptable > > to use the caller's fsuid. > > > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > > which removed the inode creation in ovl_link(). This commit move > > inode_init_owner() into ovl_create_object(), so the ovl_link() just > > give the old inode to ovl_create_or_link(). Then the override fsuid > > becomes the old inode's fsuid, neither the caller nor the overlay's > > creator! So this is incorrect. > > > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > --- > > fs/overlayfs/dir.c | 16 +++++++++++----- > > fs/overlayfs/overlayfs.h | 2 ++ > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > index 6b03457f72bb..dd84e6fc5f6e 100644 > > --- a/fs/overlayfs/dir.c > > +++ b/fs/overlayfs/dir.c > > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > > err = -ENOMEM; > > override_cred = prepare_creds(); > > if (override_cred) { > > - override_cred->fsuid = inode->i_uid; > > - override_cred->fsgid = inode->i_gid; > > + override_cred->fsuid = attr->fsuid; > > + override_cred->fsgid = attr->fsgid; > > if (!attr->hardlink) { > > err = security_dentry_create_files_as(dentry, > > attr->mode, &dentry->d_name, old_cred, > > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > > attr.mode = inode->i_mode; > > > > + attr.fsuid = inode->i_uid; > > + attr.fsgid = inode->i_gid; > > err = ovl_create_or_link(dentry, inode, &attr, false); > > /* Did we end up using the preallocated inode? */ > > if (inode != d_inode(dentry)) > > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > { > > int err; > > struct inode *inode; > > + struct ovl_cattr attr; > > > > err = ovl_want_write(old); > > if (err) > > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > inode = d_inode(old); > > ihold(inode); > > > > - err = ovl_create_or_link(new, inode, > > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > > - ovl_type_origin(old)); > > + attr = (struct ovl_cattr) { > > + .hardlink = ovl_dentry_upper(old), > > + .fsuid = current_fsuid(), > > + .fsgid = current_fsgid(), > > + }; > > Why do we need to override fsuid/fsgid for the hardlink case? > > Wouldn't it be simpler to just use the mounter's creds unmodified in > this case? The inode is not created in this case, so overriding with > current uid/gid is not necessary, I think. > > Another way to look at it is: rename(A, B) is equivalent to an > imaginary atomic [link(A, B) + unlink(A)] pair. But we don't override > uid/gid for rename() or unlink() so why should we for link(). So my assumption has been that we want to override for any creation request and so for the sake of consistency I would've expected to also use that for ->link(). Plus, this is also what has been done before the commit 51f7e52dc943 ("ovl: share inode for hard link") iiuc. Fwiw, I would've opted for consistency and even use the caller's fs{g,u}id during ->rename() and ->unlink(). Right now the caller's fs{g,u}id - indirectly through inode_init_owner() - is used to ensure that the ownership of newly created files in the upper layer are based on the caller's not on the mounter's fs{g,u}id afaict. If we continue to only override for those cases it would really help that we document this in a good comment in ovl_create_or_link(). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-31 13:43 ` Christian Brauner @ 2022-08-31 13:53 ` Miklos Szeredi 2022-08-31 15:29 ` Christian Brauner 0 siblings, 1 reply; 8+ messages in thread From: Miklos Szeredi @ 2022-08-31 13:53 UTC (permalink / raw) To: Christian Brauner Cc: Zhang Tianci, linux-unionfs, linux-kernel, amir73il, Jiachen Zhang On Wed, 31 Aug 2022 at 15:43, Christian Brauner <brauner@kernel.org> wrote: > > On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote: > > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci > > <zhangtianci.1997@bytedance.com> wrote: > > > > > > There is a wrong case of link() on overlay: > > > $ mkdir /lower /fuse /merge > > > $ mount -t fuse /fuse > > > $ mkdir /fuse/upper /fuse/work > > > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > > > workdir=work > > > $ touch /merge/file > > > $ chown bin.bin /merge/file // the file's caller becomes "bin" > > > $ ln /merge/file /merge/lnkfile > > > > > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > > > caller is "bin", it denied this request. > > > > > > In the changing history of ovl_link(), there are two key commits: > > > > > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > > > overrides the cred's fsuid/fsgid using the new inode. The new inode's > > > owner is initialized by inode_init_owner(), and inode->fsuid is > > > assigned to the current user. So the override fsuid becomes the > > > current user. We know link() is actually modifying the directory, so > > > the caller must have the MAY_WRITE permission on the directory. The > > > current caller may should have this permission. This is acceptable > > > to use the caller's fsuid. > > > > > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > > > which removed the inode creation in ovl_link(). This commit move > > > inode_init_owner() into ovl_create_object(), so the ovl_link() just > > > give the old inode to ovl_create_or_link(). Then the override fsuid > > > becomes the old inode's fsuid, neither the caller nor the overlay's > > > creator! So this is incorrect. > > > > > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > > > > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > > > > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > > --- > > > fs/overlayfs/dir.c | 16 +++++++++++----- > > > fs/overlayfs/overlayfs.h | 2 ++ > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > > index 6b03457f72bb..dd84e6fc5f6e 100644 > > > --- a/fs/overlayfs/dir.c > > > +++ b/fs/overlayfs/dir.c > > > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > > > err = -ENOMEM; > > > override_cred = prepare_creds(); > > > if (override_cred) { > > > - override_cred->fsuid = inode->i_uid; > > > - override_cred->fsgid = inode->i_gid; > > > + override_cred->fsuid = attr->fsuid; > > > + override_cred->fsgid = attr->fsgid; > > > if (!attr->hardlink) { > > > err = security_dentry_create_files_as(dentry, > > > attr->mode, &dentry->d_name, old_cred, > > > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > > > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > > > attr.mode = inode->i_mode; > > > > > > + attr.fsuid = inode->i_uid; > > > + attr.fsgid = inode->i_gid; > > > err = ovl_create_or_link(dentry, inode, &attr, false); > > > /* Did we end up using the preallocated inode? */ > > > if (inode != d_inode(dentry)) > > > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > { > > > int err; > > > struct inode *inode; > > > + struct ovl_cattr attr; > > > > > > err = ovl_want_write(old); > > > if (err) > > > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > inode = d_inode(old); > > > ihold(inode); > > > > > > - err = ovl_create_or_link(new, inode, > > > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > > > - ovl_type_origin(old)); > > > + attr = (struct ovl_cattr) { > > > + .hardlink = ovl_dentry_upper(old), > > > + .fsuid = current_fsuid(), > > > + .fsgid = current_fsgid(), > > > + }; > > > > Why do we need to override fsuid/fsgid for the hardlink case? > > > > Wouldn't it be simpler to just use the mounter's creds unmodified in > > this case? The inode is not created in this case, so overriding with > > current uid/gid is not necessary, I think. > > > > Another way to look at it is: rename(A, B) is equivalent to an > > imaginary atomic [link(A, B) + unlink(A)] pair. But we don't override > > uid/gid for rename() or unlink() so why should we for link(). > > So my assumption has been that we want to override for any creation > request and so for the sake of consistency I would've expected to also > use that for ->link(). But link() is *not* a creation op. It's a namespace manipulation op. > Plus, this is also what has been done before the > commit 51f7e52dc943 ("ovl: share inode for hard link") iiuc. It wouldn't have mattered back then either, since the upper inode was linked and not copied. > Fwiw, I would've opted for consistency and even use the caller's > fs{g,u}id during ->rename() and ->unlink(). > > Right now the caller's fs{g,u}id - indirectly through inode_init_owner() > - is used to ensure that the ownership of newly created files in the > upper layer are based on the caller's not on the mounter's fs{g,u}id > afaict. If we continue to only override for those cases it would really > help that we document this in a good comment in ovl_create_or_link(). Yep. Thanks, Miklos ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-31 13:53 ` Miklos Szeredi @ 2022-08-31 15:29 ` Christian Brauner 2022-09-01 3:02 ` 天赐张 0 siblings, 1 reply; 8+ messages in thread From: Christian Brauner @ 2022-08-31 15:29 UTC (permalink / raw) To: Miklos Szeredi Cc: Zhang Tianci, linux-unionfs, linux-kernel, amir73il, Jiachen Zhang On Wed, Aug 31, 2022 at 03:53:58PM +0200, Miklos Szeredi wrote: > On Wed, 31 Aug 2022 at 15:43, Christian Brauner <brauner@kernel.org> wrote: > > > > On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote: > > > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci > > > <zhangtianci.1997@bytedance.com> wrote: > > > > > > > > There is a wrong case of link() on overlay: > > > > $ mkdir /lower /fuse /merge > > > > $ mount -t fuse /fuse > > > > $ mkdir /fuse/upper /fuse/work > > > > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > > > > workdir=work > > > > $ touch /merge/file > > > > $ chown bin.bin /merge/file // the file's caller becomes "bin" > > > > $ ln /merge/file /merge/lnkfile > > > > > > > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > > > > caller is "bin", it denied this request. > > > > > > > > In the changing history of ovl_link(), there are two key commits: > > > > > > > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > > > > overrides the cred's fsuid/fsgid using the new inode. The new inode's > > > > owner is initialized by inode_init_owner(), and inode->fsuid is > > > > assigned to the current user. So the override fsuid becomes the > > > > current user. We know link() is actually modifying the directory, so > > > > the caller must have the MAY_WRITE permission on the directory. The > > > > current caller may should have this permission. This is acceptable > > > > to use the caller's fsuid. > > > > > > > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > > > > which removed the inode creation in ovl_link(). This commit move > > > > inode_init_owner() into ovl_create_object(), so the ovl_link() just > > > > give the old inode to ovl_create_or_link(). Then the override fsuid > > > > becomes the old inode's fsuid, neither the caller nor the overlay's > > > > creator! So this is incorrect. > > > > > > > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > > > > > > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > > > > > > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > > > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > > > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > > > --- > > > > fs/overlayfs/dir.c | 16 +++++++++++----- > > > > fs/overlayfs/overlayfs.h | 2 ++ > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > > > index 6b03457f72bb..dd84e6fc5f6e 100644 > > > > --- a/fs/overlayfs/dir.c > > > > +++ b/fs/overlayfs/dir.c > > > > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > > > > err = -ENOMEM; > > > > override_cred = prepare_creds(); > > > > if (override_cred) { > > > > - override_cred->fsuid = inode->i_uid; > > > > - override_cred->fsgid = inode->i_gid; > > > > + override_cred->fsuid = attr->fsuid; > > > > + override_cred->fsgid = attr->fsgid; > > > > if (!attr->hardlink) { > > > > err = security_dentry_create_files_as(dentry, > > > > attr->mode, &dentry->d_name, old_cred, > > > > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > > > > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > > > > attr.mode = inode->i_mode; > > > > > > > > + attr.fsuid = inode->i_uid; > > > > + attr.fsgid = inode->i_gid; > > > > err = ovl_create_or_link(dentry, inode, &attr, false); > > > > /* Did we end up using the preallocated inode? */ > > > > if (inode != d_inode(dentry)) > > > > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > > { > > > > int err; > > > > struct inode *inode; > > > > + struct ovl_cattr attr; > > > > > > > > err = ovl_want_write(old); > > > > if (err) > > > > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > > inode = d_inode(old); > > > > ihold(inode); > > > > > > > > - err = ovl_create_or_link(new, inode, > > > > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > > > > - ovl_type_origin(old)); > > > > + attr = (struct ovl_cattr) { > > > > + .hardlink = ovl_dentry_upper(old), > > > > + .fsuid = current_fsuid(), > > > > + .fsgid = current_fsgid(), > > > > + }; > > > > > > Why do we need to override fsuid/fsgid for the hardlink case? > > > > > > Wouldn't it be simpler to just use the mounter's creds unmodified in > > > this case? The inode is not created in this case, so overriding with > > > current uid/gid is not necessary, I think. > > > > > > Another way to look at it is: rename(A, B) is equivalent to an > > > imaginary atomic [link(A, B) + unlink(A)] pair. But we don't override > > > uid/gid for rename() or unlink() so why should we for link(). > > > > So my assumption has been that we want to override for any creation > > request and so for the sake of consistency I would've expected to also > > use that for ->link(). > > But link() is *not* a creation op. It's a namespace manipulation op. Yeah, I know what you mean but it's borderline in so far as the underlying fs might still perform permission checking based on the caller's fs{g,u}id which together with what I'm saying below in a bit was what made me go oh well, we should use the caller's fs{g,u}id then for consistency. > > > Plus, this is also what has been done before the > > commit 51f7e52dc943 ("ovl: share inode for hard link") iiuc. > > It wouldn't have mattered back then either, since the upper inode was > linked and not copied. What I meant was back then even for link the fs{g,u}id was based on a newly allocated inode->i_{g,u}id that was initialized through inode_init_owner() with the caller's fs{g,u}id: inode = ovl_new_inode(dentry->d_sb, mode); if (!inode) goto out; err = ovl_copy_up(dentry->d_parent); if (err) goto out_iput; inode_init_owner(inode, dentry->d_parent->d_inode, mode); stat.mode = inode->i_mode; old_cred = ovl_override_creds(dentry->d_sb); err = -ENOMEM; override_cred = prepare_creds(); if (override_cred) { override_cred->fsuid = inode->i_uid; override_cred->fsgid = inode->i_gid; and it changed after that commit to be based on old inode. So emulating the old behavior seemed the better approach. > > > Fwiw, I would've opted for consistency and even use the caller's > > fs{g,u}id during ->rename() and ->unlink(). > > > > Right now the caller's fs{g,u}id - indirectly through inode_init_owner() > > - is used to ensure that the ownership of newly created files in the > > upper layer are based on the caller's not on the mounter's fs{g,u}id > > afaict. If we continue to only override for those cases it would really > > help that we document this in a good comment in ovl_create_or_link(). > > Yep. Sounds good. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() 2022-08-31 15:29 ` Christian Brauner @ 2022-09-01 3:02 ` 天赐张 0 siblings, 0 replies; 8+ messages in thread From: 天赐张 @ 2022-09-01 3:02 UTC (permalink / raw) To: Christian Brauner Cc: Miklos Szeredi, overlayfs, linux-kernel, Amir Goldstein, Jiachen Zhang Christian Brauner <brauner@kernel.org> 于2022年8月31日周三 23:29写道： > > On Wed, Aug 31, 2022 at 03:53:58PM +0200, Miklos Szeredi wrote: > > On Wed, 31 Aug 2022 at 15:43, Christian Brauner <brauner@kernel.org> wrote: > > > > > > On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote: > > > > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci > > > > <zhangtianci.1997@bytedance.com> wrote: > > > > > > > > > > There is a wrong case of link() on overlay: > > > > > $ mkdir /lower /fuse /merge > > > > > $ mount -t fuse /fuse > > > > > $ mkdir /fuse/upper /fuse/work > > > > > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > > > > > workdir=work > > > > > $ touch /merge/file > > > > > $ chown bin.bin /merge/file // the file's caller becomes "bin" > > > > > $ ln /merge/file /merge/lnkfile > > > > > > > > > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > > > > > caller is "bin", it denied this request. > > > > > > > > > > In the changing history of ovl_link(), there are two key commits: > > > > > > > > > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > > > > > overrides the cred's fsuid/fsgid using the new inode. The new inode's > > > > > owner is initialized by inode_init_owner(), and inode->fsuid is > > > > > assigned to the current user. So the override fsuid becomes the > > > > > current user. We know link() is actually modifying the directory, so > > > > > the caller must have the MAY_WRITE permission on the directory. The > > > > > current caller may should have this permission. This is acceptable > > > > > to use the caller's fsuid. > > > > > > > > > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > > > > > which removed the inode creation in ovl_link(). This commit move > > > > > inode_init_owner() into ovl_create_object(), so the ovl_link() just > > > > > give the old inode to ovl_create_or_link(). Then the override fsuid > > > > > becomes the old inode's fsuid, neither the caller nor the overlay's > > > > > creator! So this is incorrect. > > > > > > > > > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > > > > > > > > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > > > > > > > > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com> > > > > > Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com> > > > > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > > > > --- > > > > > fs/overlayfs/dir.c | 16 +++++++++++----- > > > > > fs/overlayfs/overlayfs.h | 2 ++ > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > > > > index 6b03457f72bb..dd84e6fc5f6e 100644 > > > > > --- a/fs/overlayfs/dir.c > > > > > +++ b/fs/overlayfs/dir.c > > > > > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > > > > > err = -ENOMEM; > > > > > override_cred = prepare_creds(); > > > > > if (override_cred) { > > > > > - override_cred->fsuid = inode->i_uid; > > > > > - override_cred->fsgid = inode->i_gid; > > > > > + override_cred->fsuid = attr->fsuid; > > > > > + override_cred->fsgid = attr->fsgid; > > > > > if (!attr->hardlink) { > > > > > err = security_dentry_create_files_as(dentry, > > > > > attr->mode, &dentry->d_name, old_cred, > > > > > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > > > > > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > > > > > attr.mode = inode->i_mode; > > > > > > > > > > + attr.fsuid = inode->i_uid; > > > > > + attr.fsgid = inode->i_gid; > > > > > err = ovl_create_or_link(dentry, inode, &attr, false); > > > > > /* Did we end up using the preallocated inode? */ > > > > > if (inode != d_inode(dentry)) > > > > > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > > > { > > > > > int err; > > > > > struct inode *inode; > > > > > + struct ovl_cattr attr; > > > > > > > > > > err = ovl_want_write(old); > > > > > if (err) > > > > > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > > > inode = d_inode(old); > > > > > ihold(inode); > > > > > > > > > > - err = ovl_create_or_link(new, inode, > > > > > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > > > > > - ovl_type_origin(old)); > > > > > + attr = (struct ovl_cattr) { > > > > > + .hardlink = ovl_dentry_upper(old), > > > > > + .fsuid = current_fsuid(), > > > > > + .fsgid = current_fsgid(), > > > > > + }; > > > > > > > > Why do we need to override fsuid/fsgid for the hardlink case? > > > > > > > > Wouldn't it be simpler to just use the mounter's creds unmodified in > > > > this case? The inode is not created in this case, so overriding with > > > > current uid/gid is not necessary, I think. > > > > > > > > Another way to look at it is: rename(A, B) is equivalent to an > > > > imaginary atomic [link(A, B) + unlink(A)] pair. But we don't override > > > > uid/gid for rename() or unlink() so why should we for link(). > > > > > > So my assumption has been that we want to override for any creation > > > request and so for the sake of consistency I would've expected to also > > > use that for ->link(). > > > > But link() is *not* a creation op. It's a namespace manipulation op. > > Yeah, I know what you mean but it's borderline in so far as the > underlying fs might still perform permission checking based on the > caller's fs{g,u}id which together with what I'm saying below in a bit > was what made me go oh well, we should use the caller's fs{g,u}id then > for consistency. > > > > > > Plus, this is also what has been done before the > > > commit 51f7e52dc943 ("ovl: share inode for hard link") iiuc. > > > > It wouldn't have mattered back then either, since the upper inode was > > linked and not copied. > > What I meant was back then even for link the fs{g,u}id was based on a > newly allocated inode->i_{g,u}id that was initialized through > inode_init_owner() with the caller's fs{g,u}id: > > inode = ovl_new_inode(dentry->d_sb, mode); > if (!inode) > goto out; > > err = ovl_copy_up(dentry->d_parent); > if (err) > goto out_iput; > > inode_init_owner(inode, dentry->d_parent->d_inode, mode); > stat.mode = inode->i_mode; > > old_cred = ovl_override_creds(dentry->d_sb); > err = -ENOMEM; > override_cred = prepare_creds(); > if (override_cred) { > override_cred->fsuid = inode->i_uid; > override_cred->fsgid = inode->i_gid; > > and it changed after that commit to be based on old inode. So emulating > the old behavior seemed the better approach. > > > > > > Fwiw, I would've opted for consistency and even use the caller's > > > fs{g,u}id during ->rename() and ->unlink(). > > > > > > Right now the caller's fs{g,u}id - indirectly through inode_init_owner() > > > - is used to ensure that the ownership of newly created files in the > > > upper layer are based on the caller's not on the mounter's fs{g,u}id > > > afaict. If we continue to only override for those cases it would really > > > help that we document this in a good comment in ovl_create_or_link(). > > > > Yep. > > Sounds good. It looks like you've reached an agreement. So I will send the v3 patch using the mounter's fs{g,u}id with the proper comment in ovl_create_or_link(). Thanks, Tianci ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-01 3:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-25 13:05 [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() Zhang Tianci 2022-08-25 13:25 ` Christian Brauner 2022-08-30 2:44 ` [External] " 天赐张 2022-08-31 13:00 ` Miklos Szeredi 2022-08-31 13:43 ` Christian Brauner 2022-08-31 13:53 ` Miklos Szeredi 2022-08-31 15:29 ` Christian Brauner 2022-09-01 3:02 ` 天赐张
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).