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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h8uvgkjh.fsf@xmission.com>
Date:   Thu, 19 Oct 2017 11:38:26 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Christian Brauner <christian.brauner@...onical.com>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        linux-kernel@...r.kernel.org, stgraber@...ntu.com, tycho@...ho.ws,
        serge@...lyn.com
Subject: Re: [PATCH 1/2 v3] user namespace: use union in {g,u}idmap struct

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

> On Wed, Oct 18, 2017 at 07:48:14PM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian.brauner@...onical.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