| Message ID | 20200410145528.318074-1-christian@brauner.io |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,UNSTABLE] UBUNTU: SAUCE: shiftfs: record correct creator credentials | expand |
On Fri, Apr 10, 2020 at 04:55:28PM +0200, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@ubuntu.com> > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > When shiftfs is nested we failed to be able to create any files or > access directories because we recorded the wrong creator credentials. We > need to record the credentials of the creator of the lowers mark mount > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > the nesting case. This is similar to how we always record the user > namespace of the base filesystem. > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> This looks right to me. Acked-by: Seth Forshee <seth.forshee@canonical.com> Not sure why you only sent this for unstable though. Shouldn't it also go back to E/F?
On Fri, Apr 10, 2020 at 10:06:32AM -0500, Seth Forshee wrote: > On Fri, Apr 10, 2020 at 04:55:28PM +0200, Christian Brauner wrote: > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > > > When shiftfs is nested we failed to be able to create any files or > > access directories because we recorded the wrong creator credentials. We > > need to record the credentials of the creator of the lowers mark mount > > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > > the nesting case. This is similar to how we always record the user > > namespace of the base filesystem. > > > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > This looks right to me. > > Acked-by: Seth Forshee <seth.forshee@canonical.com> > > Not sure why you only sent this for unstable though. Shouldn't it also > go back to E/F? Yes, it should go to all LTS kernels that do have shiftfs. The reason I only put UNSTABLE in the subject is because I developed in on top of it and it has HEAD~1 as base commit from unstable. Otherwise the patch should just apply cleanly to all LTS. Next time I can just tag all of them in the subject. Thanks! Christian
On Fri, Apr 10, 2020 at 05:14:28PM +0200, Christian Brauner wrote: > On Fri, Apr 10, 2020 at 10:06:32AM -0500, Seth Forshee wrote: > > On Fri, Apr 10, 2020 at 04:55:28PM +0200, Christian Brauner wrote: > > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > > > > > When shiftfs is nested we failed to be able to create any files or > > > access directories because we recorded the wrong creator credentials. We > > > need to record the credentials of the creator of the lowers mark mount > > > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > > > the nesting case. This is similar to how we always record the user > > > namespace of the base filesystem. > > > > > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > > > This looks right to me. > > > > Acked-by: Seth Forshee <seth.forshee@canonical.com> > > > > Not sure why you only sent this for unstable though. Shouldn't it also > > go back to E/F? > > Yes, it should go to all LTS kernels that do have shiftfs. The reason I > only put UNSTABLE in the subject is because I developed in on top of it > and it has HEAD~1 as base commit from unstable. Otherwise the patch > should just apply cleanly to all LTS. Next time I can just tag all of > them in the subject. Yeah, I mean we do appreciate it if you confirm the patch applies at minimum :-) But I don't think we've let shiftfs diverge between releases, so it should be fine.
On 10.04.20 16:55, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@ubuntu.com> > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > When shiftfs is nested we failed to be able to create any files or > access directories because we recorded the wrong creator credentials. We > need to record the credentials of the creator of the lowers mark mount > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > the nesting case. This is similar to how we always record the user > namespace of the base filesystem. > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> For Focal as well: Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Should this be applied to Eoan? Eoan has shiftfs but it's not an LTS. Thanks, Kleber > --- > fs/shiftfs.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 3623d02b061e..5c39529d0a17 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -2020,6 +2020,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > * parent mark mountpoint. > */ > sbinfo->passthrough_mark = sbinfo_mp->passthrough_mark; > + sbinfo->creator_cred = get_cred(sbinfo_mp->creator_cred); > } else { > sbinfo->mnt = mntget(path.mnt); > dentry = dget(path.dentry); > @@ -2028,16 +2029,16 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > * are identical. > */ > sbinfo->passthrough_mark = sbinfo->passthrough; > - } > > - cred_tmp = prepare_creds(); > - if (!cred_tmp) { > - err = -ENOMEM; > - goto out_put_path; > + cred_tmp = prepare_creds(); > + if (!cred_tmp) { > + err = -ENOMEM; > + goto out_put_path; > + } > + /* Don't override disk quota limits or use reserved space. */ > + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > + sbinfo->creator_cred = cred_tmp; > } > - /* Don't override disk quota limits or use reserved space. */ > - cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > - sbinfo->creator_cred = cred_tmp; > } else { > /* > * This leg executes if we're admin capable in the namespace, > > base-commit: eb813ecd261d2c4c592bd4116aaa93f33c9ec4bd >
On Thu, Apr 23, 2020 at 03:12:01PM +0200, Kleber Souza wrote: > On 10.04.20 16:55, Christian Brauner wrote: > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > > > When shiftfs is nested we failed to be able to create any files or > > access directories because we recorded the wrong creator credentials. We > > need to record the credentials of the creator of the lowers mark mount > > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > > the nesting case. This is similar to how we always record the user > > namespace of the base filesystem. > > > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > For Focal as well: > > Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > > > Should this be applied to Eoan? Eoan has shiftfs but it's not an LTS. Yes, sorry. This should basically go to any versions that we still support with shiftfs. Thank! Christian > > Thanks, > Kleber > > > --- > > fs/shiftfs.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > > index 3623d02b061e..5c39529d0a17 100644 > > --- a/fs/shiftfs.c > > +++ b/fs/shiftfs.c > > @@ -2020,6 +2020,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > > * parent mark mountpoint. > > */ > > sbinfo->passthrough_mark = sbinfo_mp->passthrough_mark; > > + sbinfo->creator_cred = get_cred(sbinfo_mp->creator_cred); > > } else { > > sbinfo->mnt = mntget(path.mnt); > > dentry = dget(path.dentry); > > @@ -2028,16 +2029,16 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > > * are identical. > > */ > > sbinfo->passthrough_mark = sbinfo->passthrough; > > - } > > > > - cred_tmp = prepare_creds(); > > - if (!cred_tmp) { > > - err = -ENOMEM; > > - goto out_put_path; > > + cred_tmp = prepare_creds(); > > + if (!cred_tmp) { > > + err = -ENOMEM; > > + goto out_put_path; > > + } > > + /* Don't override disk quota limits or use reserved space. */ > > + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > > + sbinfo->creator_cred = cred_tmp; > > } > > - /* Don't override disk quota limits or use reserved space. */ > > - cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > > - sbinfo->creator_cred = cred_tmp; > > } else { > > /* > > * This leg executes if we're admin capable in the namespace, > > > > base-commit: eb813ecd261d2c4c592bd4116aaa93f33c9ec4bd > > >
On 10.04.20 16:55, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@ubuntu.com> > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > When shiftfs is nested we failed to be able to create any files or > access directories because we recorded the wrong creator credentials. We > need to record the credentials of the creator of the lowers mark mount > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > the nesting case. This is similar to how we always record the user > namespace of the base filesystem. > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > fs/shiftfs.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 3623d02b061e..5c39529d0a17 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -2020,6 +2020,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > * parent mark mountpoint. > */ > sbinfo->passthrough_mark = sbinfo_mp->passthrough_mark; > + sbinfo->creator_cred = get_cred(sbinfo_mp->creator_cred); > } else { > sbinfo->mnt = mntget(path.mnt); > dentry = dget(path.dentry); > @@ -2028,16 +2029,16 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > * are identical. > */ > sbinfo->passthrough_mark = sbinfo->passthrough; > - } > > - cred_tmp = prepare_creds(); > - if (!cred_tmp) { > - err = -ENOMEM; > - goto out_put_path; > + cred_tmp = prepare_creds(); > + if (!cred_tmp) { > + err = -ENOMEM; > + goto out_put_path; > + } > + /* Don't override disk quota limits or use reserved space. */ > + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > + sbinfo->creator_cred = cred_tmp; > } > - /* Don't override disk quota limits or use reserved space. */ > - cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); > - sbinfo->creator_cred = cred_tmp; > } else { > /* > * This leg executes if we're admin capable in the namespace, > > base-commit: eb813ecd261d2c4c592bd4116aaa93f33c9ec4bd > Applied to eoan/linux and focal/linux. Thanks, Kleber
On Fri, Apr 10, 2020 at 04:55:28PM +0200, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@ubuntu.com> > > BugLink: https://bugs.launchpad.net/bugs/1872094 > > When shiftfs is nested we failed to be able to create any files or > access directories because we recorded the wrong creator credentials. We > need to record the credentials of the creator of the lowers mark mount > of shiftfs. Otherwise we aren't privileged wrt to the shiftfs layer in > the nesting case. This is similar to how we always record the user > namespace of the base filesystem. > > Suggested-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Applied to unstable/master, thanks!
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index 3623d02b061e..5c39529d0a17 100644 --- a/fs/shiftfs.c +++ b/fs/shiftfs.c @@ -2020,6 +2020,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, * parent mark mountpoint. */ sbinfo->passthrough_mark = sbinfo_mp->passthrough_mark; + sbinfo->creator_cred = get_cred(sbinfo_mp->creator_cred); } else { sbinfo->mnt = mntget(path.mnt); dentry = dget(path.dentry); @@ -2028,16 +2029,16 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, * are identical. */ sbinfo->passthrough_mark = sbinfo->passthrough; - } - cred_tmp = prepare_creds(); - if (!cred_tmp) { - err = -ENOMEM; - goto out_put_path; + cred_tmp = prepare_creds(); + if (!cred_tmp) { + err = -ENOMEM; + goto out_put_path; + } + /* Don't override disk quota limits or use reserved space. */ + cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); + sbinfo->creator_cred = cred_tmp; } - /* Don't override disk quota limits or use reserved space. */ - cap_lower(cred_tmp->cap_effective, CAP_SYS_RESOURCE); - sbinfo->creator_cred = cred_tmp; } else { /* * This leg executes if we're admin capable in the namespace,