Re: [PATCH 1/2 v3] user namespace: use union in {g,u}idmap struct

[ebiederm@xmission.com (Eric W. Biederman)]

Christian Brauner <christian.brauner@canonical.com> writes:

> On Wed, Oct 18, 2017 at 07:48:14PM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian.brauner@canonical.com> writes:
>> 
>> > I'm not sure why the build is complaining about how the union is initialized
>> > here. This looks legitimate to me and I can't reproduce this locally with or
>> > without the appended config. The struct introduced here is:
>> >
>> > #define UID_GID_MAP_MAX_EXTENTS 5
>> >
>> > struct uid_gid_extent {
>> > 	u32 first;
>> > 	u32 lower_first;
>> > 	u32 count;
>> > };
>> >
>> > struct uid_gid_map { /* 64 bytes -- 1 cache line */
>> > 	u32 nr_extents;
>> > 	union {
>> > 		struct uid_gid_extent extent[UID_GID_MAP_MAX_EXTENTS];
>> > 		struct {
>> > 			struct uid_gid_extent *forward;
>> > 			struct uid_gid_extent *reverse;
>> > 		};
>> > 	};
>> > };
>> >
>> > And the initialization in kernel/user.c which I didn't change looks correct.
>> > But maybe I'm missing the point.
>> 
>> You may want to check your compiler version this feels like a compiler
>> dependent error.
>> 
>> It looks like gcc isn't happy about not having braces for the anonymous
>> union of extent and the anonymouns structure that holds forward and
>> reverse.
>> 
>> FYI since I am commenting.  I took a quick skim through your code today
>> and at first glance everything looks good.  The performance is nice and
>> fast, and the changes look reasonable at first glance.
>
> Thanks. Glad to hear.
>
>> 
>> I think there are some nits that can be picked but nothing yet that
>> indicates the code is working incorrectly.
>
> Do you want me to wait for your feedback? If not I'd send a new version of the
> patch with an additonal patch for kernel/user.c to use enclosing brackets when
> initializing the union in the struct.

Please do.

The only solid feedback I have at this point is that you don't need
to take userns_state_mutex on free.   As there are no references at that
point a lock isn't going to make a difference.

I think I may have seen a few extra smp_rmb() in there.  But I have not
looked closely enough to confirm that.

But all of those are the code works, there is just a little room for
improvement kind of things.  There is nothing in there (except the
kernel/user.c initialization) that I have seen so far that says it could
not be merged now.

Eric