enable building user namespace with xfs

PATCH Superseded Dwight Engen 5 participants 16 messages Jul 30, 2013 – Aug 11, 2013

lore

Share & tools

lore

Dwight Engen

author

2013-07-30 03:07 UTC #0 lore raw

PATCH v7 7/7 enable building user namespace with xfs

Quoted 1 line

From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001

From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs

Signed-off-by Dwight Engen <[email protected]>

---
 init/Kconfig | 13 -------------
 1 file changed, 13 deletions(-)

init/Kconfig +0 ~~-13~~

index 247084b..a7bcd87 100644
@@ -1106,7 +1106,6 @@ config IPC_NS
 
 config USER_NS
 	bool "User namespace"
-	depends on UIDGID_CONVERTED
 	select UIDGID_STRICT_TYPE_CHECKS
 
 	default n

@@ -1140,20 +1139,8 @@ config NET_NS
 
 endif # NAMESPACES
 
-config UIDGID_CONVERTED
-	# True if all of the selected software conmponents are known
-	# to have uid_t and gid_t converted to kuid_t and kgid_t
-	# where appropriate and are otherwise safe to use with
-	# the user namespace.
-	bool
-	default y
-
-	# Filesystems
-	depends on XFS_FS = n
-
 config UIDGID_STRICT_TYPE_CHECKS
 	bool "Require conversions between uid/gids and their internal representation"
-	depends on UIDGID_CONVERTED
 	default n
 	help
 	 While the nececessary conversions are being added to all subsystems this option allows

Signature · 6 lines

1.8.1.4

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2013-07-30 23:40 UTC #1 lore raw

Hey Dwight,

Quoted 6 lines · Dwight Engen

Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>

Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?

Thanks,
Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2013-07-31 00:21 UTC #2 lore raw

Quoted 4 lines · Ben Myers

Hey Dwight,
Quoted 1 line · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001

Quoted 8 lines

Quoted 5 lines
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?

Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.

The only thing I think we still need to address is whether xfs_ioc_setattr() should allow users within a namespace to change the project ID of a file they otherwise own. That function is currently changed to use a inode_owner_or_capable() check and so if the uids match inside the namespace the modification is allowed.

However, right now for project IDs I think we have decided to limit manipulations to the init user namespace and not expose project IDs inside user namespaces at all. Hence I think that xfs_ioc_setattr() needs a further check for this...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2013-07-31 13:25 UTC #3 lore raw

Hey,

Quoted 9 lines · Dave Chinner

Quoted 8 lines · Ben Myers
Quoted 6 lines · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>

Quoted 7 lines

Quoted 2 lines
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.

Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.

Quoted 10 lines

The only thing I think we still need to address is whether xfs_ioc_setattr() should allow users within a namespace to change the project ID of a file they otherwise own. That function is currently changed to use a inode_owner_or_capable() check and so if the uids match inside the namespace the modification is allowed.
However, right now for project IDs I think we have decided to limit manipulations to the init user namespace and not expose project IDs inside user namespaces at all. Hence I think that xfs_ioc_setattr() needs a further check for this...

Looks like that should be added to patch 3. Dwight, if you prefer to repost only patch 3 that's fine.

-Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dwight Engen

author

2013-07-31 17:09 UTC #4 lore raw

Quoted 17 lines · On Wed, 31 Jul 2013 08:25:23 -0500 Ben Myers <[email protected]> wrote:

Hey,
Quoted 14 lines · Dave Chinner
Quoted 12 lines · Ben Myers
Quoted 7 lines · Dwight Engen
Quoted 2 lines
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?

Quoted 7 lines

Quoted 4 lines
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.

Hi Ben, a process can be CAP_SYS_ADMIN in its own userns, but the check for that would be ns_capable(CAP_SYS_ADMIN) not capable(CAP_SYS_ADMIN) which checks against init_user_ns. The checks Dave was mentioning are all against init_user_ns so bulkstat and open-by-handle effectively cannot be used from inside a non-init_user_ns.

Quoted 6 lines

Quoted 6 lines
The only thing I think we still need to address is whether xfs_ioc_setattr() should allow users within a namespace to change the project ID of a file they otherwise own. That function is currently changed to use a inode_owner_or_capable() check and so if the uids match inside the namespace the modification is allowed.

Quoted 7 lines

Quoted 4 lines
However, right now for project IDs I think we have decided to limit manipulations to the init user namespace and not expose project IDs inside user namespaces at all. Hence I think that xfs_ioc_setattr() needs a further check for this...
Looks like that should be added to patch 3. Dwight, if you prefer to repost only patch 3 that's fine.

I'll add this into patch 3 once we get it sorted out and then repost with Gao's reviewed-by tags. Thanks.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2013-07-31 23:29 UTC #5 lore raw

In reply to Ben Myers

Quoted 15 lines · Ben Myers

Hey,
Quoted 12 lines · Dave Chinner
Quoted 10 lines · Ben Myers
Quoted 6 lines · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?

Quoted 7 lines

Quoted 4 lines
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.

Ben, until about a week and a half ago I was also working under that same understanding as you. So don't feel bad about not knowing about this basic, fundamental rule because it is completely undocumented and it's not obvious to anyone reading the code until someone points it out....

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Ben Myers

2013-08-01 15:06 UTC #6 lore raw

Dave,

Quoted 21 lines · Dave Chinner

Quoted 20 lines · Ben Myers
Hey,
Quoted 16 lines · Dave Chinner
Quoted 10 lines · Ben Myers
Quoted 6 lines · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.

Quoted 5 lines

Quoted 2 lines
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.
Ben, until about a week and a half ago I was also working under that same understanding as you.

Well huh. According to Dwight you can have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns. Kinda cool, IMO.

-Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dwight Engen

author

2013-08-01 16:17 UTC #7 lore raw

Quoted 29 lines · On Thu, 1 Aug 2013 10:06:43 -0500 Ben Myers <[email protected]> wrote:

Dave,
Quoted 26 lines · Dave Chinner
Quoted 24 lines · Ben Myers
Hey,
Quoted 10 lines · Dave Chinner
Quoted 9 lines · Ben Myers
Quoted 7 lines · Dwight Engen
Quoted 2 lines
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Quoted 8 lines
Quoted 3 lines
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.

Quoted 6 lines

Quoted 2 lines
Ben, until about a week and a half ago I was also working under that same understanding as you.
Well huh. According to Dwight you can have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns. Kinda cool, IMO.

Yeah, see commit 3486740a and cap_capable(). Took me a bit to grok that for loop, but I think the semantics are that if you are in a non init_user_ns, you can start from current_user_ns() looking for a cap and walk back the heirarchy (userns's can be nested) but it won't ever check in init_user_ns, and so by definition you have no capabilities in init_user_ns.

ie. if you are in a userns, you might be nsown_capable(CAP_SYS_ADMIN) but you cannot ever be capable(CAP_SYS_ADMIN). Thats my understanding anyway :)

Quoted 1 line

-Ben

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Serge E. Hallyn

2013-08-06 15:11 UTC #8 lore raw

Quoted 35 lines · Dwight Engen

Quoted 32 lines · On Thu, 1 Aug 2013 10:06:43 -0500 Ben Myers <[email protected]> wrote:
Dave,
Quoted 28 lines · Dave Chinner
Quoted 17 lines · Ben Myers
Hey,
Quoted 14 lines · Dave Chinner
Quoted 12 lines · Ben Myers
Quoted 7 lines · Dwight Engen
Quoted 2 lines
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Quoted 7 lines
Quoted 4 lines
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.
Ben, until about a week and a half ago I was also working under that same understanding as you.

Quoted 7 lines

Quoted 3 lines
Well huh. According to Dwight you can have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns. Kinda cool, IMO.
Yeah, see commit 3486740a and cap_capable(). Took me a bit to grok that for loop, but I think the semantics are that if you are in a non init_user_ns, you can start from current_user_ns() looking for a cap

To be precise, you're not looking for a cap in the parent ns. You're checking whether the subject is the creator of one of the object's namespace's parents.

Quoted 3 lines

and walk back the heirarchy (userns's can be nested) but it won't ever check in init_user_ns, and so by definition you have no capabilities in init_user_ns.

You also have no caps in any of your parent namespaces. The loop is checking over the *object*'s parents to see if the subject owns them. I'ts not looping over the *subject*'s parents, as they do not grant any privilege. Only the subject's bottom-most credentials grant privilege.

Quoted 3 lines

ie. if you are in a userns, you might be nsown_capable(CAP_SYS_ADMIN) but you cannot ever be capable(CAP_SYS_ADMIN). Thats my understanding anyway :)

Yup.

-serge

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Serge E. Hallyn

2013-08-07 14:59 UTC #9 lore raw

In reply to Dave Chinner

Quoted 21 lines · Dave Chinner

Quoted 20 lines · Ben Myers
Hey,
Quoted 16 lines · Dave Chinner
Quoted 10 lines · Ben Myers
Quoted 6 lines · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.

Quoted 8 lines

Quoted 2 lines
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.
Ben, until about a week and a half ago I was also working under that same understanding as you. So don't feel bad about not knowing about this basic, fundamental rule because it is completely undocumented and it's not obvious to anyone reading the code until someone points it out....

It's actually all documented in new manpages like namespaces(7) and user_namespaces(7). Unfortunately those don't seem to have been released yet.

Michael, I don't see any git development/experimental git branch at kernel.org/pub/scm/docs/man-pages - do you have those pages up somewhere?

thanks,
-serge

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Serge E. Hallyn

2013-08-07 15:01 UTC #10 lore raw

Gah, forgot to actually add Michael to the cc:, sorry.

Quoted 38 lines · Serge E. Hallyn

Quoted 21 lines · Dave Chinner
Quoted 20 lines · Ben Myers
Hey,
Quoted 16 lines · Dave Chinner
Quoted 10 lines · Ben Myers
Quoted 6 lines · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.
Quoted 8 lines
Quoted 2 lines
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.
Ben, until about a week and a half ago I was also working under that same understanding as you. So don't feel bad about not knowing about this basic, fundamental rule because it is completely undocumented and it's not obvious to anyone reading the code until someone points it out....
It's actually all documented in new manpages like namespaces(7) and user_namespaces(7). Unfortunately those don't seem to have been released yet.
Michael, I don't see any git development/experimental git branch at kernel.org/pub/scm/docs/man-pages - do you have those pages up somewhere?

Quoted 7 lines

thanks,
-serge
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2013-08-11 23:57 UTC #11 lore raw

***** SUSPECTED SPAM ***** Re: [PATCH v7 7/7] enable building user namespace with xfs

In reply to Serge E. Hallyn

Quoted 25 lines · Serge E. Hallyn

Quoted 24 lines · Dave Chinner
Quoted 22 lines · Ben Myers
Hey,
Quoted 9 lines · Dave Chinner
Quoted 8 lines · Ben Myers
Quoted 6 lines · Dwight Engen
Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
Quoted 7 lines
Quoted 2 lines
Was there a patch running around to limit bulkstat to init_user_ns? Any other items that needed to be addressed before applying this patch?
Bulkstat has a capable(CAP_SYS_ADMIN) check and therefore can only be executed in the init name space. Similarly, all the open-by-handle interfaces have the same capable() checks so they can only be executed int he init name space, too.
Gah. I was under the impression that you could have a process with CAP_SYS_ADMIN in a namespace other than init_user_ns.

Quoted 8 lines

Quoted 5 lines
Ben, until about a week and a half ago I was also working under that same understanding as you. So don't feel bad about not knowing about this basic, fundamental rule because it is completely undocumented and it's not obvious to anyone reading the code until someone points it out....
It's actually all documented in new manpages like namespaces(7) and user_namespaces(7). Unfortunately those don't seem to have been released yet.

User facing documentation goes in man pages.

My comments about the above point at the fact that there is no developer facing documentation that tell us how to safely and *securely* implement namespace support in different filesystems. Information on the architecture, design and use of internal kernel infrastructure for kernel developers should be in the Documentation/ subdirectory of the kernel tree.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dwight Engen

author

2013-07-31 18:19 UTC #12 lore raw

In reply to Dave Chinner

On Wed, 31 Jul 2013 10:21:19 +1000
Dave Chinner <[email protected]> wrote:

[...]

Quoted 5 lines

The only thing I think we still need to address is whether xfs_ioc_setattr() should allow users within a namespace to change the project ID of a file they otherwise own. That function is currently changed to use a inode_owner_or_capable() check and so if the uids match inside the namespace the modification is allowed.

Right, so before this change the caller has to own the file or be CAP_FOWNER in init_user_ns. Changing to using inode_owner_or_capable() means the caller has to own the file (and the inode's uid must be valid in current_user_ns) or be CAP_FOWNER in current_user_ns. I don't see how this lets the user the user do something inside the userns that they can't do outside.

Basically I think we want to treat projids as a non namespace aware identifier, but it sounds like maybe we don't want them manipulated from userns context at all.

Quoted 4 lines

However, right now for project IDs I think we have decided to limit manipulations to the init user namespace and not expose project IDs inside user namespaces at all. Hence I think that xfs_ioc_setattr() needs a further check for this...

If we don't want to allow setting the projid from a userns, the check I would propose inside the if (mask & FSX_EXTSIZE) block is:
if (current_user_ns() != &init_user_ns)
Is the appropriate error return for this EINVAL? (thats what a similar check in kernel/taskstats.c returns)

Quoted 3 lines

Cheers,
Dave.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2013-07-31 23:44 UTC #13 lore raw

Quoted 10 lines · Dwight Engen

On Wed, 31 Jul 2013 10:21:19 +1000
Dave Chinner <[email protected]> wrote:
[...]
Quoted 5 lines
The only thing I think we still need to address is whether xfs_ioc_setattr() should allow users within a namespace to change the project ID of a file they otherwise own. That function is currently changed to use a inode_owner_or_capable() check and so if the uids match inside the namespace the modification is allowed.

Quoted 6 lines

Right, so before this change the caller has to own the file or be CAP_FOWNER in init_user_ns. Changing to using inode_owner_or_capable() means the caller has to own the file (and the inode's uid must be valid in current_user_ns) or be CAP_FOWNER in current_user_ns. I don't see how this lets the user the user do something inside the userns that they can't do outside.

Right.

Quoted 3 lines

Basically I think we want to treat projids as a non namespace aware identifier, but it sounds like maybe we don't want them manipulated from userns context at all.

Exactly.

Quoted 10 lines

Quoted 4 lines
However, right now for project IDs I think we have decided to limit manipulations to the init user namespace and not expose project IDs inside user namespaces at all. Hence I think that xfs_ioc_setattr() needs a further check for this...
If we don't want to allow setting the projid from a userns, the check I would propose inside the if (mask & FSX_EXTSIZE) block is:
if (current_user_ns() != &init_user_ns)
Is the appropriate error return for this EINVAL? (thats what a similar check in kernel/taskstats.c returns)

Yeah, it looks like the very few cases where an error is returned for such a check it is an EINVAL case. Don't forget a comment explaining this.

We probably also fix getxstate/getxquota implementations to reject project quota requests as well. It might be best to do this with an additional patch.

As it is, we're going to need to revisit the project quota support in fs/quota/quota.c because that maps project IDs to/from user namespaces. This was done based on the overriding assertion of the userns implementor that project quota IDs must be mapped via namespaces, but as we are getting down to it the details (i.e. project IDs only visible in the init user ns) we can see that this is incorrect and we're going to need to do quite a bit of surgery there to fix this up....

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Gao feng

2013-08-01 00:53 UTC #14 lore raw

Quoted 45 lines · Dave Chinner

Quoted 10 lines · Dwight Engen
On Wed, 31 Jul 2013 10:21:19 +1000
Dave Chinner <[email protected]> wrote:
[...]
Quoted 5 lines
The only thing I think we still need to address is whether xfs_ioc_setattr() should allow users within a namespace to change the project ID of a file they otherwise own. That function is currently changed to use a inode_owner_or_capable() check and so if the uids match inside the namespace the modification is allowed.
Quoted 6 lines
Right, so before this change the caller has to own the file or be CAP_FOWNER in init_user_ns. Changing to using inode_owner_or_capable() means the caller has to own the file (and the inode's uid must be valid in current_user_ns) or be CAP_FOWNER in current_user_ns. I don't see how this lets the user the user do something inside the userns that they can't do outside.
Right.
Quoted 3 lines
Basically I think we want to treat projids as a non namespace aware identifier, but it sounds like maybe we don't want them manipulated from userns context at all.
Exactly.
Quoted 10 lines
Quoted 4 lines
However, right now for project IDs I think we have decided to limit manipulations to the init user namespace and not expose project IDs inside user namespaces at all. Hence I think that xfs_ioc_setattr() needs a further check for this...
If we don't want to allow setting the projid from a userns, the check I would propose inside the if (mask & FSX_EXTSIZE) block is:
if (current_user_ns() != &init_user_ns)
Is the appropriate error return for this EINVAL? (thats what a similar check in kernel/taskstats.c returns)
Yeah, it looks like the very few cases where an error is returned for such a check it is an EINVAL case. Don't forget a comment explaining this.
We probably also fix getxstate/getxquota implementations to reject project quota requests as well. It might be best to do this with an additional patch.

Quoted 8 lines

As it is, we're going to need to revisit the project quota support in fs/quota/quota.c because that maps project IDs to/from user namespaces. This was done based on the overriding assertion of the userns implementor that project quota IDs must be mapped via namespaces, but as we are getting down to it the details (i.e. project IDs only visible in the init user ns) we can see that this is incorrect and we're going to need to do quite a bit of surgery there to fix this up....

Yes, in the implementation of userns, project ids can be modified and project quota is allowed to setup in container safely, but compared with removing xfs dependence, I hope this dependence to be removed firstly.

the mapping project IDs to user namespace work should be fixed after we finish this removing dependence work. :)

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Gao feng

Reviewed-by on v7 7/7

2013-07-31 07:19 UTC #15 lore raw

In reply to Dwight Engen

Quoted 7 lines · Dwight Engen

Quoted 1 line
From e6a9ee0cfa0ed40484f66bc1726dc19de36038b8 Mon Sep 17 00:00:00 2001
From: Dwight Engen <[email protected]>
Date: Tue, 2 Jul 2013 09:52:54 -0400
Subject: [PATCH 7/7] enable building user namespace with xfs
Signed-off-by Dwight Engen <[email protected]>
---

looks good to me, thanks :)

Reviewed-by Gao feng <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs