From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752326AbdKANF2 (ORCPT ); Wed, 1 Nov 2017 09:05:28 -0400 Received: from mx2.suse.de ([195.135.220.15]:53512 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751853AbdKANF1 (ORCPT ); Wed, 1 Nov 2017 09:05:27 -0400 Subject: Re: [PATCH 3/5] userns: Don't read extents twice in m_start To: "Eric W. Biederman" , Nikolay Borisov Cc: Christian Brauner , tycho@tycho.ws, Linux Containers , linux-kernel@vger.kernel.org, Joe Azzopardi , joe@perches.com, apw@canonical.com, "Paul E. McKenney" , Peter Zijlstra , mathieu.desnoyers@efficios.com References: <20171024220441.10235-1-christian.brauner@ubuntu.com> <20171024220441.10235-2-christian.brauner@ubuntu.com> <871sliubhj.fsf_-_@xmission.com> <87k1zaswu6.fsf_-_@xmission.com> <143adb61-fb8e-fc1b-396b-b18836e68766@suse.com> <87a806ntn0.fsf@xmission.com> From: Nikolay Borisov Message-ID: Date: Wed, 1 Nov 2017 15:05:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <87a806ntn0.fsf@xmission.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [CCing some memory-barriers people + checkpatch maintainers] On 1.11.2017 13:08, Eric W. Biederman wrote: > Nikolay Borisov writes: > >> On 1.11.2017 01:48, Eric W. Biederman wrote: >>> >>> This is important so reading /proc//{uid_map,gid_map,projid_map} while >>> the map is being written does not do strange things. >>> >>> Signed-off-by: "Eric W. Biederman" >>> --- >>> 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@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/containers >