Hi all,
After merging the final tree, today's linux-next build (powerpc
allyesconfig) failed like this:
arch/powerpc/platforms/cell/spufs/inode.c: In function 'spufs_parse_options':
arch/powerpc/platforms/cell/spufs/inode.c:623:16: error: incompatible types when assigning to type 'kuid_t' from type 'int'
root->i_uid = option;
^
arch/powerpc/platforms/cell/spufs/inode.c:628:16: error: incompatible types when assigning to type 'kgid_t' from type 'int'
root->i_gid = option;
^
Caused by commit d6970d4b726c ("enable building user namespace with xfs")
from the xfs tree (that was fun to find :-)).
I have reverted that commit for today. It could probably be replaced
with a patch that just changed XFS_FS to SPU_FS in the UIDGID_CONVERTED
config dependency - or someone could fix up SPU_FS.
--
Cheers,
Stephen Rothwell [email protected]
On Tue, 20 Aug 2013 17:20:52 +1000
Stephen Rothwell <[email protected]> wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> arch/powerpc/platforms/cell/spufs/inode.c: In function
> 'spufs_parse_options':
> arch/powerpc/platforms/cell/spufs/inode.c:623:16: error: incompatible
> types when assigning to type 'kuid_t' from type 'int' root->i_uid =
> option; ^ arch/powerpc/platforms/cell/spufs/inode.c:628:16: error:
> incompatible types when assigning to type 'kgid_t' from type 'int'
> root->i_gid = option; ^
>
> Caused by commit d6970d4b726c ("enable building user namespace with
> xfs") from the xfs tree (that was fun to find :-)).
>
> I have reverted that commit for today. It could probably be replaced
> with a patch that just changed XFS_FS to SPU_FS in the
> UIDGID_CONVERTED config dependency - or someone could fix up SPU_FS.
Hi, (already sent this email based on Intel's kbuild robot this
morning, sorry for the dup to those who already got it).
Yep this looks to me like SPU_FS should have been in the list of
stuff that had not been UIDGID_CONVERTED, but reviving
UIDGID_CONVERTED and adding SPU_FS to it won't work for
non powerpc arch because SPU_FS = n won't be defined. The following can
be used to mark it as incompatible with USER_NS:
diff --git a/arch/powerpc/platforms/cell/Kconfig
b/arch/powerpc/platforms/cell/Kconfig index 9978f59..fcf8336 100644
--- a/arch/powerpc/platforms/cell/Kconfig
+++ b/arch/powerpc/platforms/cell/Kconfig
@@ -61,6 +61,7 @@ config SPU_FS
tristate "SPU file system"
default m
depends on PPC_CELL
+ depends on USER_NS=n
select SPU_BASE
select MEMORY_HOTPLUG
help
Or if the rest of spufs is already okay for user namespace (I have not
checked it, but this seems to be the only place it is dealing with
uid/gid), then the following will fix these particular errors
(cross-compile tested, but I don't have a powerpc to run test on):
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index f390042..90fb308 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -620,12 +620,12 @@ spufs_parse_options(struct super_block *sb, char *options, struct inode *root)
case Opt_uid:
if (match_int(&args[0], &option))
return 0;
- root->i_uid = option;
+ root->i_uid = make_kuid(&init_user_ns, option);
break;
case Opt_gid:
if (match_int(&args[0], &option))
return 0;
- root->i_gid = option;
+ root->i_gid = make_kgid(&init_user_ns, option);
break;
case Opt_mode:
if (match_octal(&args[0], &option))
Hi Jeremy,
Apologies for breaking your build...
On Tue, Aug 20, 2013 at 12:07:02PM -0400, Dwight Engen wrote:
> On Tue, 20 Aug 2013 17:20:52 +1000
> Stephen Rothwell <[email protected]> wrote:
> > After merging the final tree, today's linux-next build (powerpc
> > allyesconfig) failed like this:
> >
> > arch/powerpc/platforms/cell/spufs/inode.c: In function
> > 'spufs_parse_options':
> > arch/powerpc/platforms/cell/spufs/inode.c:623:16: error: incompatible
> > types when assigning to type 'kuid_t' from type 'int' root->i_uid =
> > option; ^ arch/powerpc/platforms/cell/spufs/inode.c:628:16: error:
> > incompatible types when assigning to type 'kgid_t' from type 'int'
> > root->i_gid = option; ^
> >
> > Caused by commit d6970d4b726c ("enable building user namespace with
> > xfs") from the xfs tree (that was fun to find :-)).
> >
> > I have reverted that commit for today. It could probably be replaced
> > with a patch that just changed XFS_FS to SPU_FS in the
> > UIDGID_CONVERTED config dependency - or someone could fix up SPU_FS.
>
> Hi, (already sent this email based on Intel's kbuild robot this
> morning, sorry for the dup to those who already got it).
>
> Yep this looks to me like SPU_FS should have been in the list of
> stuff that had not been UIDGID_CONVERTED, but reviving
> UIDGID_CONVERTED and adding SPU_FS to it won't work for
> non powerpc arch because SPU_FS = n won't be defined. The following can
> be used to mark it as incompatible with USER_NS:
>
> diff --git a/arch/powerpc/platforms/cell/Kconfig
> b/arch/powerpc/platforms/cell/Kconfig index 9978f59..fcf8336 100644
> --- a/arch/powerpc/platforms/cell/Kconfig
> +++ b/arch/powerpc/platforms/cell/Kconfig
> @@ -61,6 +61,7 @@ config SPU_FS
> tristate "SPU file system"
> default m
> depends on PPC_CELL
> + depends on USER_NS=n
> select SPU_BASE
> select MEMORY_HOTPLUG
> help
>
> Or if the rest of spufs is already okay for user namespace (I have not
> checked it, but this seems to be the only place it is dealing with
> uid/gid), then the following will fix these particular errors
> (cross-compile tested, but I don't have a powerpc to run test on):
>
> diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
> index f390042..90fb308 100644
> --- a/arch/powerpc/platforms/cell/spufs/inode.c
> +++ b/arch/powerpc/platforms/cell/spufs/inode.c
> @@ -620,12 +620,12 @@ spufs_parse_options(struct super_block *sb, char *options, struct inode *root)
> case Opt_uid:
> if (match_int(&args[0], &option))
> return 0;
> - root->i_uid = option;
> + root->i_uid = make_kuid(&init_user_ns, option);
> break;
> case Opt_gid:
> if (match_int(&args[0], &option))
> return 0;
> - root->i_gid = option;
> + root->i_gid = make_kgid(&init_user_ns, option);
> break;
> case Opt_mode:
> if (match_octal(&args[0], &option))
I'd prefer not to break Stephen's tree two days in a row. We could just revert
d6970d4b726c in the xfs tree for the time being as Stephen has done, but given
the choice would prefer the fix. Do you have a preference between the two
approaches that Dwight has posted? The first seems more conservative...
Thanks,
Ben
On Tuesday 20 August 2013, Dwight Engen wrote:
> diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
> index f390042..90fb308 100644
> --- a/arch/powerpc/platforms/cell/spufs/inode.c
> +++ b/arch/powerpc/platforms/cell/spufs/inode.c
> @@ -620,12 +620,12 @@ spufs_parse_options(struct super_block *sb, char *options, struct inode *root)
> case Opt_uid:
> if (match_int(&args[0], &option))
> return 0;
> - root->i_uid = option;
> + root->i_uid = make_kuid(&init_user_ns, option);
> break;
> case Opt_gid:
> if (match_int(&args[0], &option))
> return 0;
> - root->i_gid = option;
> + root->i_gid = make_kgid(&init_user_ns, option);
> break;
> case Opt_mode:
> if (match_octal(&args[0], &option))
Doesn't this mean the uid/gid is taken from the initial namespace rather than
from the namespace of the 'mount' process calling this? I think the logical
choice would be to have the UID be the one that gets passed here in the caller's
namespace.
Arnd
Hi Ben,
On Tue, 20 Aug 2013 14:28:44 -0500 Ben Myers <[email protected]> wrote:
>
> I'd prefer not to break Stephen's tree two days in a row. We could just revert
> d6970d4b726c in the xfs tree for the time being as Stephen has done, but given
> the choice would prefer the fix. Do you have a preference between the two
> approaches that Dwight has posted? The first seems more conservative...
I will automatically revert that commit when I merge the xfs tree until
some other solution is forthcoming (so you don't have to do the revert in
the xfs tree). This does need to be fixed fairly soon, though.
--
Cheers,
Stephen Rothwell [email protected]
On Tue, 20 Aug 2013 22:46:30 +0200
Arnd Bergmann <[email protected]> wrote:
> On Tuesday 20 August 2013, Dwight Engen wrote:
> > diff --git a/arch/powerpc/platforms/cell/spufs/inode.c
> > b/arch/powerpc/platforms/cell/spufs/inode.c index f390042..90fb308
> > 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c
> > +++ b/arch/powerpc/platforms/cell/spufs/inode.c
> > @@ -620,12 +620,12 @@ spufs_parse_options(struct super_block *sb,
> > char *options, struct inode *root) case Opt_uid:
> > if (match_int(&args[0], &option))
> > return 0;
> > - root->i_uid = option;
> > + root->i_uid = make_kuid(&init_user_ns,
> > option); break;
> > case Opt_gid:
> > if (match_int(&args[0], &option))
> > return 0;
> > - root->i_gid = option;
> > + root->i_gid = make_kgid(&init_user_ns,
> > option); break;
> > case Opt_mode:
> > if (match_octal(&args[0], &option))
>
> Doesn't this mean the uid/gid is taken from the initial namespace
> rather than from the namespace of the 'mount' process calling this? I
> think the logical choice would be to have the UID be the one that
> gets passed here in the caller's namespace.
Yes, I agree. The other filesystems that take an Opt_uid as well do use
current_user_ns() and not init_user_ns. They also do a uid_valid()
check and fail the mount (or fallback to GLOBAL_ROOT_UID). So I think
that would look like this:
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index f390042..87ba7cf 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -620,12 +620,16 @@ spufs_parse_options(struct super_block *sb, char *options, struct inode *root)
case Opt_uid:
if (match_int(&args[0], &option))
return 0;
- root->i_uid = option;
+ root->i_uid = make_kuid(current_user_ns(), option);
+ if (!uid_valid(root->i_uid))
+ return 0;
break;
case Opt_gid:
if (match_int(&args[0], &option))
return 0;
- root->i_gid = option;
+ root->i_gid = make_kgid(current_user_ns(), option);
+ if (!gid_valid(root->i_gid))
+ return 0;
break;
case Opt_mode:
if (match_octal(&args[0], &option))
Again, I have not run tested this so we may just want to disable SPU_FS
with USER_NS until they can be tested together.
Hi all,
> Yes, I agree. The other filesystems that take an Opt_uid as well do use
> current_user_ns() and not init_user_ns. They also do a uid_valid()
> check and fail the mount (or fallback to GLOBAL_ROOT_UID). So I think
> that would look like this:
Looks good to me. Builds and mounts as expected.
Acked-by: Jeremy Kerr <[email protected]>
Cheers,
Jeremy
Hey Stephen,
On Wed, Aug 21, 2013 at 10:22:46AM +1000, Stephen Rothwell wrote:
> On Tue, 20 Aug 2013 14:28:44 -0500 Ben Myers <[email protected]> wrote:
> > I'd prefer not to break Stephen's tree two days in a row. We could just revert
> > d6970d4b726c in the xfs tree for the time being as Stephen has done, but given
> > the choice would prefer the fix. Do you have a preference between the two
> > approaches that Dwight has posted? The first seems more conservative...
>
> I will automatically revert that commit when I merge the xfs tree until
> some other solution is forthcoming (so you don't have to do the revert in
> the xfs tree).
Gah. That makes sense. ;)
> This does need to be fixed fairly soon, though.
Agreed, thanks.
-Ben
Hey Dwight,
On Wed, Aug 21, 2013 at 02:30:04PM +0800, Jeremy Kerr wrote:
> > Yes, I agree. The other filesystems that take an Opt_uid as well do use
> > current_user_ns() and not init_user_ns. They also do a uid_valid()
> > check and fail the mount (or fallback to GLOBAL_ROOT_UID). So I think
> > that would look like this:
>
> Looks good to me. Builds and mounts as expected.
>
> Acked-by: Jeremy Kerr <[email protected]>
Could you repost this patch with the right subject and a commit header? Given
Jeremy's Ack I think we could proceed to pull this in.
Regards,
Ben
Acked-by: Jeremy Kerr <[email protected]>
Tested-by: Jeremy Kerr <[email protected]>
Signed-off-by: Dwight Engen <[email protected]>
---
arch/powerpc/platforms/cell/spufs/inode.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index f390042..87ba7cf 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -620,12 +620,16 @@ spufs_parse_options(struct super_block *sb, char *options, struct inode *root)
case Opt_uid:
if (match_int(&args[0], &option))
return 0;
- root->i_uid = option;
+ root->i_uid = make_kuid(current_user_ns(), option);
+ if (!uid_valid(root->i_uid))
+ return 0;
break;
case Opt_gid:
if (match_int(&args[0], &option))
return 0;
- root->i_gid = option;
+ root->i_gid = make_kgid(current_user_ns(), option);
+ if (!gid_valid(root->i_gid))
+ return 0;
break;
case Opt_mode:
if (match_octal(&args[0], &option))
--
1.8.1.4
On Wed, 21 Aug 2013 10:56:54 -0500
Ben Myers <[email protected]> wrote:
> Hey Dwight,
>
> On Wed, Aug 21, 2013 at 02:30:04PM +0800, Jeremy Kerr wrote:
> > > Yes, I agree. The other filesystems that take an Opt_uid as well
> > > do use current_user_ns() and not init_user_ns. They also do a
> > > uid_valid() check and fail the mount (or fallback to
> > > GLOBAL_ROOT_UID). So I think that would look like this:
> >
> > Looks good to me. Builds and mounts as expected.
> >
> > Acked-by: Jeremy Kerr <[email protected]>
>
> Could you repost this patch with the right subject and a commit
> header? Given Jeremy's Ack I think we could proceed to pull this in.
Sure, I just wanted to make sure someone had tested it first, which it
looks like Jeremy did, thanks.
> Regards,
> Ben
On Wednesday 21 August 2013, Dwight Engen wrote:
>
> Acked-by: Jeremy Kerr <[email protected]>
> Tested-by: Jeremy Kerr <[email protected]>
> Signed-off-by: Dwight Engen <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
On Wed, Aug 21, 2013 at 10:05:27PM +0200, Arnd Bergmann wrote:
> On Wednesday 21 August 2013, Dwight Engen wrote:
> >
> > Acked-by: Jeremy Kerr <[email protected]>
> > Tested-by: Jeremy Kerr <[email protected]>
> > Signed-off-by: Dwight Engen <[email protected]>
>
> Reviewed-by: Arnd Bergmann <[email protected]>
Applied.
Thanks,
-Ben