Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580938 > unrolled thread

Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Vivek Goyal <vgoyal@redhat.com> - 2017-02-15 00:10 +0100
    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-15 00:50 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Vivek Goyal <vgoyal@redhat.com> - 2017-02-15 15:20 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-16 17:00 +0100
          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Vivek Goyal <vgoyal@redhat.com> - 2017-02-16 17:50 +0100
            Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-16 18:00 +0100
              Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount ebiederm@xmission.com (Eric W. Biederman) - 2017-02-17 03:10 +0100
                Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Djalal Harouni <tixxdz@gmail.com> - 2017-02-17 09:40 +0100
                Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-17 18:30 +0100
                  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount ebiederm@xmission.com (Eric W. Biederman) - 2017-02-20 05:40 +0100
                    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-22 13:10 +0100

#1580938 — Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

On Sun, Feb 05, 2017 at 05:18:11PM -0800, James Bottomley wrote:

[..]
> >  shiftfs is going to miss out on overlayfs bug fixes related to user 
> > credentials differ from mounter credentials, like fd3220d ("ovl: 
> > update S_ISGID when setting posix ACLs"). I am not sure that this 
> > specific case is relevant to shiftfs, but there could be other.
> 
> OK, so shiftfs doesn't have this bug and the reason why is
> illustrative: basically shiftfs does three things
> 
>    1. lookups via a uid/gid shifted dentry cache
>    2. shifted credential inode operations permission checks on the
>       underlying filesystem
>    3. location marking for unprivileged mount
> 
> I think we've already seen that 1. isn't from overlayfs but the
> functionality could be added to overlayfs, I suppose.  The big problem
> is 2.  The overlayfs code emulates the permission checks, which makes
> it rather complex (this is where you get your bugs like the above
> from).  I did actually look at adding 2. to overlayfs on the theory
> that a single layer overlay might be closest to what this is, but
> eventually concluded I'd have to take the special cases and add a whole
> lot more to them ... it really would increase the maintenance burden
> substantially and make the code an unreadable rats nest.

Hi James,

If we merge this functionality in overlayfs, then we could avoid extra
copy of dentry/inode and that might be a significant advantage.

W.r.t permission checks, I am wondering will it make sense to do what
overlayfs is doing for shiftfs. That is permission is checked on
two inodes. We use creds of task for checking permission on
shiftfs/overlay inode and mounter's creds on real inode.

Given we have already shifted the uid/gid for shiftfs inode, I am 
wondering that why can't we simply call generic_permission(shiftfs_inode,
mask) directly in the context of caller. Something like..

shiftfs_permission() {
  err = generic_permission(inode, mask);
  if (err)
  	return err;

  switch_to_mounter_creds;
  err = inode_permission(reali, mask);
  revert_creds();

  return err;
}

Vivek

[toc] | [next] | [standalone]

#1580948

On Tue, 2017-02-14 at 18:03 -0500, Vivek Goyal wrote:
> On Sun, Feb 05, 2017 at 05:18:11PM -0800, James Bottomley wrote:
> 
> [..]
> > >  shiftfs is going to miss out on overlayfs bug fixes related to
> > > user 
> > > credentials differ from mounter credentials, like fd3220d ("ovl: 
> > > update S_ISGID when setting posix ACLs"). I am not sure that this
> > > specific case is relevant to shiftfs, but there could be other.
> > 
> > OK, so shiftfs doesn't have this bug and the reason why is
> > illustrative: basically shiftfs does three things
> > 
> >    1. lookups via a uid/gid shifted dentry cache
> >    2. shifted credential inode operations permission checks on the
> >       underlying filesystem
> >    3. location marking for unprivileged mount
> > 
> > I think we've already seen that 1. isn't from overlayfs but the
> > functionality could be added to overlayfs, I suppose.  The big 
> > problem is 2.  The overlayfs code emulates the permission checks, 
> > which makes it rather complex (this is where you get your bugs like 
> > the above from).  I did actually look at adding 2. to overlayfs on 
> > the theory that a single layer overlay might be closest to what 
> > this is, but eventually concluded I'd have to take the special 
> > cases and add a whole lot more to them ... it really would increase 
> > the maintenance burden substantially and make the code an
> > unreadable rats nest.
> 
> Hi James,
> 
> If we merge this functionality in overlayfs, then we could avoid 
> extra copy of dentry/inode and that might be a significant advantage.

I made that argument to Viro originally when I tried to do all lookups
via the underlying cache.  In the end, it's 192 bytes per dentry and
584 per inode, all of which are reclaimable, so it's not much of an
advantage and it is a great simplification to the code.  In general if
you have a natural separation, you should make the layers reflect it.

My container use case doesn't use overlayfs currently, so to me it
wouldn't provide any advantage whatsoever.

> W.r.t permission checks, I am wondering will it make sense to do what
> overlayfs is doing for shiftfs. That is permission is checked on
> two inodes. We use creds of task for checking permission on
> shiftfs/overlay inode and mounter's creds on real inode.

The mounter's creds for overlay are usually admin ones, so it's local
permission check asks should I? and the later one asks can I? (as in
would my original admin creds allow this).  In many ways, overlayfs is
ignoring the fact that the underlying ->permissions() call might have
failed for some good reason on the current creds.  I don't think any
serious trouble results from this but it strikes me as icky.

> Given we have already shifted the uid/gid for shiftfs inode, I am 
> wondering that why can't we simply call generic_permission(shiftfs_in
> ode, mask) directly in the context of caller. Something like..
> 
> shiftfs_permission() {
>   err = generic_permission(inode, mask);
>   if (err)
>   	return err;
> 
>   switch_to_mounter_creds;
>   err = inode_permission(reali, mask);
>   revert_creds();
> 
>   return err;
> }

Because if the reali->d_iop->permission exists, you should use it.  You
could argue shiftfs_permission should be

	if (iop->permission) {
		oldcred = shiftfs_new_creds(&newcred, inode->i_sb);
		err = iop->permission(reali, mask);
		shiftfs_old_creds(oldcred, &newcred);
	} else
		err = generic_permission(inode, mask);

But really that's a small optimisation.

James

[toc] | [prev] | [next] | [standalone]

#1581341

On Tue, Feb 14, 2017 at 03:45:55PM -0800, James Bottomley wrote:
> On Tue, 2017-02-14 at 18:03 -0500, Vivek Goyal wrote:
> > On Sun, Feb 05, 2017 at 05:18:11PM -0800, James Bottomley wrote:
> > 
> > [..]
> > > >  shiftfs is going to miss out on overlayfs bug fixes related to
> > > > user 
> > > > credentials differ from mounter credentials, like fd3220d ("ovl: 
> > > > update S_ISGID when setting posix ACLs"). I am not sure that this
> > > > specific case is relevant to shiftfs, but there could be other.
> > > 
> > > OK, so shiftfs doesn't have this bug and the reason why is
> > > illustrative: basically shiftfs does three things
> > > 
> > >    1. lookups via a uid/gid shifted dentry cache
> > >    2. shifted credential inode operations permission checks on the
> > >       underlying filesystem
> > >    3. location marking for unprivileged mount
> > > 
> > > I think we've already seen that 1. isn't from overlayfs but the
> > > functionality could be added to overlayfs, I suppose.  The big 
> > > problem is 2.  The overlayfs code emulates the permission checks, 
> > > which makes it rather complex (this is where you get your bugs like 
> > > the above from).  I did actually look at adding 2. to overlayfs on 
> > > the theory that a single layer overlay might be closest to what 
> > > this is, but eventually concluded I'd have to take the special 
> > > cases and add a whole lot more to them ... it really would increase 
> > > the maintenance burden substantially and make the code an
> > > unreadable rats nest.
> > 
> > Hi James,
> > 
> > If we merge this functionality in overlayfs, then we could avoid 
> > extra copy of dentry/inode and that might be a significant advantage.
> 
> I made that argument to Viro originally when I tried to do all lookups
> via the underlying cache.  In the end, it's 192 bytes per dentry and
> 584 per inode, all of which are reclaimable, so it's not much of an
> advantage and it is a great simplification to the code.  In general if
> you have a natural separation, you should make the layers reflect it.

ok.

> 
> My container use case doesn't use overlayfs currently, so to me it
> wouldn't provide any advantage whatsoever.

In docker and other use cases, this probably will be used in conjunction
with overlayfs as containers would like to write data and that should not
go back to image dir and should be sent to container specific dir.

> 
> > W.r.t permission checks, I am wondering will it make sense to do what
> > overlayfs is doing for shiftfs. That is permission is checked on
> > two inodes. We use creds of task for checking permission on
> > shiftfs/overlay inode and mounter's creds on real inode.
> 
> The mounter's creds for overlay are usually admin ones, so it's local
> permission check asks should I? and the later one asks can I? (as in
> would my original admin creds allow this).  In many ways, overlayfs is
> ignoring the fact that the underlying ->permissions() call might have
> failed for some good reason on the current creds.  I don't think any
> serious trouble results from this but it strikes me as icky.

So we do call ->permission() of underlying inode but with the creds of
mounter (as you noted). Given we don't call reali->permission() with
the creds of task, it resulted in issues with disk quota. mounter
had CAP_SYS_RESOURCE so disk quota was being ignored. But that's easily
fixable by taking away CAP_SYS_RESOURCE from mounter's creds if caller
does not have CAP_SYS_RESOURCE.

> 
> > Given we have already shifted the uid/gid for shiftfs inode, I am 
> > wondering that why can't we simply call generic_permission(shiftfs_in
> > ode, mask) directly in the context of caller. Something like..
> > 
> > shiftfs_permission() {
> >   err = generic_permission(inode, mask);
> >   if (err)
> >   	return err;
> > 
> >   switch_to_mounter_creds;
> >   err = inode_permission(reali, mask);
> >   revert_creds();
> > 
> >   return err;
> > }
> 
> Because if the reali->d_iop->permission exists, you should use it.  You
> could argue shiftfs_permission should be
> 
> 	if (iop->permission) {
> 		oldcred = shiftfs_new_creds(&newcred, inode->i_sb);
> 		err = iop->permission(reali, mask);
> 		shiftfs_old_creds(oldcred, &newcred);
> 	} else
> 		err = generic_permission(inode, mask);
> 
> But really that's a small optimisation.

ok. I thought using mounter's creds for real inode checks, will probably
do away with need of modifying caller's user namespace in
shiftfs_get_up_creds().

cred->fsuid = KUIDT_INIT(from_kuid(sb->s_user_ns, cred->fsuid));
cred->fsgid = KGIDT_INIT(from_kgid(sb->s_user_ns, cred->fsgid));
cred->user_ns = ssi->userns;

IIUC, we are shifting caller's fsuid and fsgid into caller's user
namespace but at the same time using the user_ns of reali->sb->sb_user_ns.
Feels little twisted to me. May be I am misunderstanding it.

Two levels of checks will simplify this a bit. Top level inode will belong
to the user namespace of caller and checks should pass. And mounter's
creds will have ownership over the real inode so no additional namespace
shifting required there. We could also save these creds at mount time
once and don't have to prepare it for every call. (not sure if it has
significant performance issue or not).  Just thinking aloud...

Vivek

[toc] | [prev] | [next] | [standalone]

#1582661

On Wed, 2017-02-15 at 09:17 -0500, Vivek Goyal wrote:
> On Tue, Feb 14, 2017 at 03:45:55PM -0800, James Bottomley wrote:
> > On Tue, 2017-02-14 at 18:03 -0500, Vivek Goyal wrote:

[...]
> > > Given we have already shifted the uid/gid for shiftfs inode, I am
> > > wondering that why can't we simply call
> > > generic_permission(shiftfs_inode, mask) directly in the context
> > > of caller. Something like..
> > > 
> > > shiftfs_permission() {
> > >   err = generic_permission(inode, mask);
> > >   if (err)
> > >   	return err;
> > > 
> > >   switch_to_mounter_creds;
> > >   err = inode_permission(reali, mask);
> > >   revert_creds();
> > > 
> > >   return err;
> > > }
> > 
> > Because if the reali->d_iop->permission exists, you should use it. 
> >  You could argue shiftfs_permission should be
> > 
> > 	if (iop->permission) {
> > 		oldcred = shiftfs_new_creds(&newcred, inode->i_sb);
> > 		err = iop->permission(reali, mask);
> > 		shiftfs_old_creds(oldcred, &newcred);
> > 	} else
> > 		err = generic_permission(inode, mask);
> > 
> > But really that's a small optimisation.
> 
> ok. I thought using mounter's creds for real inode checks, will 
> probably do away with need of modifying caller's user namespace in
> shiftfs_get_up_creds().

Well, no ... the mounter of a marked superblock is container admin, but
the owner in the filesystem view is real root.  The unprivileged
mounter's credentials aren't sufficient, therefore. 

> cred->fsuid = KUIDT_INIT(from_kuid(sb->s_user_ns, cred->fsuid));
> cred->fsgid = KGIDT_INIT(from_kgid(sb->s_user_ns, cred->fsgid));
> cred->user_ns = ssi->userns;
> 
> IIUC, we are shifting caller's fsuid and fsgid into caller's user
> namespace but at the same time using the user_ns of reali->sb
> ->sb_user_ns. Feels little twisted to me. May be I am
> misunderstanding it.

Actually what we're doing is shifting the credentials into the
underlying mount's filesystem view.

> Two levels of checks will simplify this a bit. Top level inode will 
> belong to the user namespace of caller and checks should pass. And 
> mounter's creds will have ownership over the real inode so no 
> additional namespace shifting required there.

That's the problem: for a marked mount, they don't.

>  We could also save these creds at mount time once and don't have to
> prepare it for every call. (not sure if it has significant
> performance issue or not).  Just thinking aloud...

If it proves to be an issue, I suppose the shift could be cached, but I
really don't think it matters that much.

James

[toc] | [prev] | [next] | [standalone]

#1582700

On Thu, Feb 16, 2017 at 07:51:58AM -0800, James Bottomley wrote:

[..]
> > Two levels of checks will simplify this a bit. Top level inode will 
> > belong to the user namespace of caller and checks should pass. And 
> > mounter's creds will have ownership over the real inode so no 
> > additional namespace shifting required there.
> 
> That's the problem: for a marked mount, they don't.

In this new model it does not fit directly. 

I was playing with a slightly different approach and modified patches so
that real root still does the mounting and takes an mount option which
specifies which user namespace we want to shift into. Thanks to Eric for
the idea.

mount -t shiftfs -o userns_fd=<fd> source shifted-fs

In this case real-root is mounter and notion of using mounter's creds on
real-inode works.

This requires a user namespace to be created before shiftfs can be mounted
and then container admin should be able to bind mount shifted-fs.

In this model, intervention of real-root is still required to setup
container and shiftfs. I guess that might not satisfy your needs where
unprivileged user should be able to launch container and be able to
make use of shiftfs, IIUC.

Vivek

[toc] | [prev] | [next] | [standalone]

#1582708

On Thu, 2017-02-16 at 11:42 -0500, Vivek Goyal wrote:
> On Thu, Feb 16, 2017 at 07:51:58AM -0800, James Bottomley wrote:
> 
> [..]
> > > Two levels of checks will simplify this a bit. Top level inode 
> > > will belong to the user namespace of caller and checks should 
> > > pass. And mounter's creds will have ownership over the real inode 
> > > so no additional namespace shifting required there.
> > 
> > That's the problem: for a marked mount, they don't.
> 
> In this new model it does not fit directly. 
> 
> I was playing with a slightly different approach and modified patches 
> so that real root still does the mounting and takes an mount option
> which specifies which user namespace we want to shift into. Thanks to 
> Eric for the idea.
> 
> mount -t shiftfs -o userns_fd=<fd> source shifted-fs

This is a non-starter because it doesn't work for the unprivileged use
case, which is what I'm really interested in.  For fully unprivileged
containers you don't have an orchestration system to ask to build the
container.  You can get init scripts to set stuff up for you, like the
marks, but ideally it should just work even without that (so an inode
flag following project semantics seems really appealing), but after
that the unprivileged user should be able to build their own
containers.

As you saw from the reply to Eric, this approach (which I have tried)
also opens up a whole can of worms for non-FS_USERNS_MOUNT filesystems.

James


> In this case real-root is mounter and notion of using mounter's creds 
> on real-inode works. 
> This requires a user namespace to be created before shiftfs can be 
> mounted and then container admin should be able to bind mount shifted
> -fs.
> 
> In this model, intervention of real-root is still required to setup
> container and shiftfs. I guess that might not satisfy your needs 
> where unprivileged user should be able to launch container and be 
> able to make use of shiftfs, IIUC.
> 
> Vivek
> 

[toc] | [prev] | [next] | [standalone]

#1583032

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Thu, 2017-02-16 at 11:42 -0500, Vivek Goyal wrote:
>> On Thu, Feb 16, 2017 at 07:51:58AM -0800, James Bottomley wrote:
>> 
>> [..]
>> > > Two levels of checks will simplify this a bit. Top level inode 
>> > > will belong to the user namespace of caller and checks should 
>> > > pass. And mounter's creds will have ownership over the real inode 
>> > > so no additional namespace shifting required there.
>> > 
>> > That's the problem: for a marked mount, they don't.
>> 
>> In this new model it does not fit directly. 
>> 
>> I was playing with a slightly different approach and modified patches 
>> so that real root still does the mounting and takes an mount option
>> which specifies which user namespace we want to shift into. Thanks to 
>> Eric for the idea.
>> 
>> mount -t shiftfs -o userns_fd=<fd> source shifted-fs

> This is a non-starter because it doesn't work for the unprivileged use
> case, which is what I'm really interested in.

But I believe it does.  It just requires a bit more work for in the
shiftfs filesystem above.  It should be perfectly possible with the help
of newuidmap to create a user namespace with the desired mappings.

My understanding is that Vivek started with requiring root to mount the
filesystem only as a simplification during development, and that the
plan is to get the basic use case working and then allow unprivileged
mounting.

> For fully unprivileged
> containers you don't have an orchestration system to ask to build the
> container.  You can get init scripts to set stuff up for you, like the
> marks, but ideally it should just work even without that (so an inode
> flag following project semantics seems really appealing), but after
> that the unprivileged user should be able to build their own
> containers.
>
> As you saw from the reply to Eric, this approach (which I have tried)
> also opens up a whole can of worms for non-FS_USERNS_MOUNT filesystems.
>

From what I can see we have two cases we care about.
A) A non-default mapping from the filesystem to the rest of the system
   and roughly s_user_ns provides that but we need a review of the
   filesystems to make certain something has not been forgotten.

B) A filesystem image sitting around in a directory somewhere that
   we want to map differently into different user namespaces while
   using the same files as backing store.

For the second case what is interesting technically is that we want
multiple mappings.  A user namespace appears adequate to specify those
extra mappings (effectively from kuids to kuids).

So we need something to associate the additional mapping with a
directory tree.  A stackable filesystem with it's own s_user_ns field
appears a very straight forward way to do that.  Especially if it can
figure out how to assert that the underlying filesystem image is
read-only (doesn't overlayfs require that?).  Making the entire stack
read-only.

I don't see a problem with that for unprivileged use (except possibly
the read-only enforcement on the unerlying directory tree).

What Vivek is talking about appears to be perfectly correct.  Performing
the underlying filesystem permission checks as the possibly unprivileged
user who mounted shiftfs.  After performing a set of permission checks
(at the shiftfs level) as the user who is accessing the files.


. . .


I think I am missing something but I completely do not understand that
subthread that says use file marks and perform the work in the vfs.
The problem is that fundamentally we need multiple mappings and I don't
see a mark on a file (even an inherited mark) providing the mapping so I
don't see the point.

Which leaves two possible places to store the extra mapping.  In the
struct mount.  Or in a stacked filesystem super_block.  For a stacked
filesystem I can see where to store the extra translation.  In the upper
filesystems upper inode.   And we can perform the practical permission
check on the upper inode as well.

For a vfs level solution it looks like we would have to change all of
the permission checking code in the kernel to have a special case for
this kind of mapping.  Which does not sound maintainable.

So at the moment I don't think a vfs level solution makes any sense.

And then if you have a stacked filesystem with FS_USERNS_MOUNT set so it
can be mounted by an unprivileged user.   I think it makes sense to
check the mounters creds agains the real inode.  To verify the user that
mounted the filesystem has the permission to perform the desired access.

Which makes only allows the mounter as much permisison as the mounter
would have if they performed the work with fuse instead of a special
in-kernel filesystem.

In a DAC model of the world that makes lots of sense.  I don't know what
actually makes sense in a MAC world.  But I am certain that is something
that can be worked through.

Eric

[toc] | [prev] | [next] | [standalone]

#1583203

On Fri, Feb 17, 2017 at 2:57 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> writes:
>
>> On Thu, 2017-02-16 at 11:42 -0500, Vivek Goyal wrote:
>>> On Thu, Feb 16, 2017 at 07:51:58AM -0800, James Bottomley wrote:
>>>
>>> [..]
>>> > > Two levels of checks will simplify this a bit. Top level inode
>>> > > will belong to the user namespace of caller and checks should
>>> > > pass. And mounter's creds will have ownership over the real inode
>>> > > so no additional namespace shifting required there.
>>> >
>>> > That's the problem: for a marked mount, they don't.
>>>
>>> In this new model it does not fit directly.
>>>
>>> I was playing with a slightly different approach and modified patches
>>> so that real root still does the mounting and takes an mount option
>>> which specifies which user namespace we want to shift into. Thanks to
>>> Eric for the idea.
>>>
>>> mount -t shiftfs -o userns_fd=<fd> source shifted-fs
>
>> This is a non-starter because it doesn't work for the unprivileged use
>> case, which is what I'm really interested in.
>
> But I believe it does.  It just requires a bit more work for in the
> shiftfs filesystem above.  It should be perfectly possible with the help
> of newuidmap to create a user namespace with the desired mappings.
>
> My understanding is that Vivek started with requiring root to mount the
> filesystem only as a simplification during development, and that the
> plan is to get the basic use case working and then allow unprivileged
> mounting.
>
>> For fully unprivileged
>> containers you don't have an orchestration system to ask to build the
>> container.  You can get init scripts to set stuff up for you, like the
>> marks, but ideally it should just work even without that (so an inode
>> flag following project semantics seems really appealing), but after
>> that the unprivileged user should be able to build their own
>> containers.
>>
>> As you saw from the reply to Eric, this approach (which I have tried)
>> also opens up a whole can of worms for non-FS_USERNS_MOUNT filesystems.
>>
>
> From what I can see we have two cases we care about.
> A) A non-default mapping from the filesystem to the rest of the system
>    and roughly s_user_ns provides that but we need a review of the
>    filesystems to make certain something has not been forgotten.
>
> B) A filesystem image sitting around in a directory somewhere that
>    we want to map differently into different user namespaces while
>    using the same files as backing store.
>
> For the second case what is interesting technically is that we want
> multiple mappings.  A user namespace appears adequate to specify those
> extra mappings (effectively from kuids to kuids).
>
> So we need something to associate the additional mapping with a
> directory tree.  A stackable filesystem with it's own s_user_ns field
> appears a very straight forward way to do that.  Especially if it can
> figure out how to assert that the underlying filesystem image is
> read-only (doesn't overlayfs require that?).  Making the entire stack
> read-only.
>
> I don't see a problem with that for unprivileged use (except possibly
> the read-only enforcement on the unerlying directory tree).
>
> What Vivek is talking about appears to be perfectly correct.  Performing
> the underlying filesystem permission checks as the possibly unprivileged
> user who mounted shiftfs.  After performing a set of permission checks
> (at the shiftfs level) as the user who is accessing the files.
>
>
> . . .
>
>
> I think I am missing something but I completely do not understand that
> subthread that says use file marks and perform the work in the vfs.
> The problem is that fundamentally we need multiple mappings and I don't
> see a mark on a file (even an inherited mark) providing the mapping so I
> don't see the point.
>
> Which leaves two possible places to store the extra mapping.  In the
> struct mount.  Or in a stacked filesystem super_block.  For a stacked
> filesystem I can see where to store the extra translation.  In the upper
> filesystems upper inode.   And we can perform the practical permission
> check on the upper inode as well.
>
> For a vfs level solution it looks like we would have to change all of
> the permission checking code in the kernel to have a special case for
> this kind of mapping.  Which does not sound maintainable.

Facts: for basic permissions: 3 files changed, 19 insertions(+), 6
deletions(-)  https://lkml.org/lkml/2016/5/4/417

That made permissions work for basically *all* filesystems. However
yes it does not handle xattr acls...

> So at the moment I don't think a vfs level solution makes any sense.
>
The permissions change was already done when userns were merged. What
you may need is VFS accessors, instead of working directly on
inode->i_uid ask the VFS to give you the right i_uid (which can also
be the case of projectid proposed by Christoph iff I got it right...)
you need it for both ways: to report to userspace and the other way to
pass it to the underlying filesystem for writes/quota which Dave
Chinner pointed out.

Any way seems the ship has settled, so my thoughts at that time were
to follow the change made for i_uid_read(), i_gid_read() helpers where
userns were merged. The code comment says: "Helper functions so that
in most cases filesystems will not need to deal directly with kuid_t
and kgid_t" so the start was from there: VFS should be the one to
handle everything using accessors for both directions. Now if you guys
think that having multiple user namespaces contexts for every
container, mount namepsace user namespace, s_user_ns and shiftfs user
ns ... or multiple APIs that will just add confusion, me I can see
this directly with orchestration/containers developers they just don't
understand what's happening... ? they want something like bind mounts!
A new filesystem is a new filesystem.

Maybe Eric you will find something useful from these comments.

Thanks!


-- 
tixxdz

[toc] | [prev] | [next] | [standalone]

#1583616

On Fri, 2017-02-17 at 14:57 +1300, Eric W. Biederman wrote:
> I think I am missing something but I completely do not understand 
> that subthread that says use file marks and perform the work in the 
> vfs. The problem is that fundamentally we need multiple mappings and 
> I don't see a mark on a file (even an inherited mark) providing the 
> mapping so I don't see the point.

The point of the mark is that it's a statement by the system
administrator that the underlying subtree is safe to be mounted by an
unprivileged container in the containers user view (i.e. with
current_user_ns() == s_user_ns).  For the unprivileged container
there's no real arbitrary s_user_ns use case because the unprivileged
container must prove it can set up the mapping, so it would likely
always be mounting from within a user_ns with the mapping it wanted.

James

[toc] | [prev] | [next] | [standalone]

#1584343

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Fri, 2017-02-17 at 14:57 +1300, Eric W. Biederman wrote:
>> I think I am missing something but I completely do not understand 
>> that subthread that says use file marks and perform the work in the 
>> vfs. The problem is that fundamentally we need multiple mappings and 
>> I don't see a mark on a file (even an inherited mark) providing the 
>> mapping so I don't see the point.
>
> The point of the mark is that it's a statement by the system
> administrator that the underlying subtree is safe to be mounted by an
> unprivileged container in the containers user view (i.e. with
> current_user_ns() == s_user_ns).  For the unprivileged container
> there's no real arbitrary s_user_ns use case because the unprivileged
> container must prove it can set up the mapping, so it would likely
> always be mounting from within a user_ns with the mapping it wanted.

As a statement that it is ok for the unprivileged mapping code to
operate that seems reasonable.  I don't currently the need for such an
ok from the system adminstrator, but if you need it a flag that
propagates to children and child directories seems reasonable.

Eric

[toc] | [prev] | [next] | [standalone]

#1586112

On Mon, 2017-02-20 at 17:24 +1300, Eric W. Biederman wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> > On Fri, 2017-02-17 at 14:57 +1300, Eric W. Biederman wrote:
> > > I think I am missing something but I completely do not understand
> > > that subthread that says use file marks and perform the work in 
> > > the vfs. The problem is that fundamentally we need multiple 
> > > mappings and I don't see a mark on a file (even an inherited 
> > > mark) providing the mapping so I don't see the point.
> > 
> > The point of the mark is that it's a statement by the system
> > administrator that the underlying subtree is safe to be mounted by 
> > an unprivileged container in the containers user view (i.e. with
> > current_user_ns() == s_user_ns).  For the unprivileged container
> > there's no real arbitrary s_user_ns use case because the 
> > unprivileged container must prove it can set up the mapping, so it 
> > would likely always be mounting from within a user_ns with the 
> > mapping it wanted.
> 
> As a statement that it is ok for the unprivileged mapping code to
> operate that seems reasonable.  I don't currently the need for such 
> an ok from the system adminstrator, but if you need it a flag that
> propagates to children and child directories seems reasonable.

The other way to do this is with an extended attribute.  I've played
around with that approach and quite like it: the advantage is that it's
sticky across system reboots; The down side is that it requires
additional VFS code to make sure you can't execute from the non-user_ns
view.

James

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web