From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752511AbbE0Ws3 (ORCPT ); Wed, 27 May 2015 18:48:29 -0400 Received: from mail-la0-f44.google.com ([209.85.215.44]:34446 "EHLO mail-la0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751630AbbE0Ws2 (ORCPT ); Wed, 27 May 2015 18:48:28 -0400 Date: Thu, 28 May 2015 01:48:25 +0300 From: Cyrill Gorcunov To: Alexey Dobriyan Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, jarod@redhat.com, jstancek@redhat.com Subject: Re: [PATCH 2/2] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline Message-ID: <20150527224825.GI17625@uranus.sw.swsoft.com> References: <20150527214757.GA12863@p183.telecom.by> <20150527214953.GB12863@p183.telecom.by> <20150527221435.GG17625@uranus.sw.swsoft.com> <20150527222941.GA28699@p183.telecom.by> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150527222941.GA28699@p183.telecom.by> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 28, 2015 at 01:29:42AM +0300, Alexey Dobriyan wrote: > > > + > > > + page = (char *)__get_free_page(GFP_TEMPORARY); > > > + if (!page) { > > > + rv = -ENOMEM; > > > + goto out_mmput; > > > + } > > > + > > > + down_read(&mm->mmap_sem); > > > + arg_start = mm->arg_start; > > > + arg_end = mm->arg_end; > > > + env_start = mm->env_start; > > > + env_end = mm->env_end; > > > + up_read(&mm->mmap_sem); > > > > Could you please explain why this down/up is needed? > > Code is written this way to get constistent snapshot of data. it does not. you fetch data into local variables which is the same as simply read them locklessly in general (because later you refer to local vars). > If you look at PR_SET_MM_* code, you'll notice down_read(&mm->mmap_sem) > as well which is a separate bug because you're _writing_ those fields > eventually in prctl_set_mm(), yuck! yes, there members are modified under read-lock and initially i didn't see any problem with that except one can have inconsistent statistics output because another process modified these fields (we validate that new members are having sane values at least in new interface and after your first patch). But now I think that down_write may be more suitable here. Cyrill