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] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 1 Nov 2017 15:05:22 +0200
From:   Nikolay Borisov <nborisov@...e.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Nikolay Borisov <nborisov@...e.com>
Cc:     Christian Brauner <christian.brauner@...ntu.com>, tycho@...ho.ws,
        Linux Containers <containers@...ts.linux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Joe Azzopardi <Joe.Azzopardi@...rofocus.com>, joe@...ches.com,
        apw@...onical.com, "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        mathieu.desnoyers@...icios.com
Subject: Re: [PATCH 3/5] userns: Don't read extents twice in m_start

[CCing some memory-barriers people + checkpatch maintainers]

On  1.11.2017 13:08, Eric W. Biederman wrote:
> Nikolay Borisov <nborisov@...e.com> writes:
> 
>> On  1.11.2017 01:48, Eric W. Biederman wrote:
>>>
>>> This is important so reading /proc/<pid>/{uid_map,gid_map,projid_map} while
>>> the map is being written does not do strange things.
>>>
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
>>> ---
>>>  kernel/user_namespace.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>>> index 563a2981d7c7..4f7e357ac1e2 100644
>>> --- a/kernel/user_namespace.c
>>> +++ b/kernel/user_namespace.c
>>> @@ -683,11 +683,13 @@ static void *m_start(struct seq_file *seq, loff_t *ppos,
>>>  		     struct uid_gid_map *map)
>>>  {
>>>  	loff_t pos = *ppos;
>>> +	unsigned extents = map->nr_extents;
>>> +	smp_rmb();
>>
>> Barriers need to be paired to work correctly as well as have explicit
>> comments describing the pairing as per kernel coding style. Checkpatch
>> will actually produce warning for that particular memory barrier.
> 
> So please look at the code and read the comment.  The fact the barrier
> was not in m_start earlier is strictly speaking a bug.

The comment you are referring to (I guess) is the one in map_write,
right? However it states nothing about which barrier pairs with which
one so it's not possible to reason with 100% certainty about the
intentions of the code. I think the fact there was a missing barrier
here just makes this point stronger.

Additionally, when someone reading the code sees a rmb being added
without a pairing wmb it's only natural to think that something is amiss
in this case.

> 
> In practice except for a very narrow window when this data is changing
> the one time it can, this code does not matter at all.
> 
> As for checkpatch I have sympathy for it, checkpatch has a hard job,
> but I won't listen to checkpatch when it is wrong.

Well if you deem the checkpatch then this rule is better removed from
checkpatch. However, considering how hard it is to debug and reason
about correctness of barriers without explicit comments I'd say you are
in the wrong on this one.

> 
> If you have additional cleanups you would like to make in this area
> please send patches.
> 
> Eric
> 
>>>  
>>> -	if (pos >= map->nr_extents)
>>> +	if (pos >= extents)
>>>  		return NULL;
>>>  
>>> -	if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
>>> +	if (extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
>>>  		return &map->extent[pos];
>>>  
>>>  	return &map->forward[pos];
>>>
> _______________________________________________
> Containers mailing list
> Containers@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ