linux-next: build failure after merge of the final tree

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.

Quoted 11 lines · On Tue, 20 Aug 2013 17:20:52 +1000 Stephen Rothwell <[email protected]>…

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; ^

Quoted 6 lines

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:

(quoted diff) +0 -0

b/arch/powerpc/platforms/cell/Kconfig index 9978f59..fcf8336 100644

arch/powerpc/platforms/cell/Kconfig +1 -0

@@ -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):

arch/powerpc/platforms/cell/spufs/inode.c +0 -1

index f390042..90fb308 100644
@@ -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;

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...

Quoted 4 lines · 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.

arch/powerpc/platforms/cell/spufs/inode.c +0 -1

index f390042..90fb308 100644
@@ -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;

Quoted 16 lines · Dwight Engen

(quoted diff) +0 -0

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

(quoted diff) +0 -0

@@ -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;

(quoted diff) +1 -1

-                       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;

(quoted diff) +1 -1

-                       root->i_gid = option;
+                       root->i_gid = make_kgid(&init_user_ns,

Quoted 3 lines

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:

Quoted 4 lines

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]>

Hey Dwight,

Quoted 5 lines · Jeremy Kerr

Quoted 4 lines

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:

Quoted 3 lines

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]>

Quoted 4 lines · Dwight Engen

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]>