From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755274AbcI2L5J (ORCPT ); Thu, 29 Sep 2016 07:57:09 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:31193 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752132AbcI2L5G (ORCPT ); Thu, 29 Sep 2016 07:57:06 -0400 Subject: Re: [PATCH v2 0/8] pipe: fix limit handling To: "Michael Kerrisk (man-pages)" , Andrew Morton References: <89e4d1b0-39b9-0262-5cd5-229766962c56@gmail.com> Cc: Willy Tarreau , socketpair@gmail.com, Tetsuo Handa , Jens Axboe , Al Viro , linux-api@vger.kernel.org, linux-kernel@vger.kernel.org From: Vegard Nossum Message-ID: Date: Thu, 29 Sep 2016 13:56:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <89e4d1b0-39b9-0262-5cd5-229766962c56@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/29/2016 02:20 AM, Michael Kerrisk (man-pages) wrote: > When changing a pipe's capacity with fcntl(F_SETPIPE_SZ), various > limits defined by /proc/sys/fs/pipe-* files are checked to see > if unprivileged users are exceeding limits on memory consumption. > > While documenting and testing the operation of these limits I noticed > that, as currently implemented, these checks have a number of problems: > > (1) When increasing the pipe capacity, the checks against the limits > in /proc/sys/fs/pipe-user-pages-{soft,hard} are made against > existing consumption, and exclude the memory required for the > increased pipe capacity. The new increase in pipe capacity can then > push the total memory used by the user for pipes (possibly far) over > a limit. This can also trigger the problem described next. > > (2) The limit checks are performed even when the new pipe capacity > is less than the existing pipe capacity. This can lead to problems > if a user sets a large pipe capacity, and then the limits are > lowered, with the result that the user will no longer be able to > decrease the pipe capacity. > > (3) As currently implemented, accounting and checking against the > limits is done as follows: > > (a) Test whether the user has exceeded the limit. > (b) Make new pipe buffer allocation. > (c) Account new allocation against the limits. > > This is racey. Multiple processes may pass point (a) simultaneously, > and then allocate pipe buffers that are accounted for only in step > (c). The race means that the user's pipe buffer allocation could be > pushed over the limit (by an arbitrary amount, depending on how > unlucky we were in the race). [Thanks to Vegard Nossum for spotting > this point, which I had missed.] > > This patch series addresses these three problems. > > Patch history: > > v1 This patch series is an improvement on a smaller series I sent > earlier to fix the user limit handling for pipes. I've made many > changes after feedback from Vegard Nossum, including the addition > of a fix for point (3) above. > > v2 Changes are noted in individual patches. > > Cc: Willy Tarreau > Cc: Vegard Nossum > Cc: socketpair@gmail.com > Cc: Tetsuo Handa > Cc: Jens Axboe > Cc: Al Viro > Cc: linux-api@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Michael Kerrisk > > Michael Kerrisk (8): > pipe: relocate round_pipe_size() above pipe_set_size() > pipe: move limit checking logic into pipe_set_size() > pipe: refactor argument for account_pipe_buffers() > pipe: fix limit checking in pipe_set_size() > pipe: simplify logic in alloc_pipe_info() > pipe: fix limit checking in alloc_pipe_info() > pipe: make account_pipe_buffers() return a value, and use it > pipe: cap initial pipe capacity according to pipe-max-size limit > > fs/pipe.c | 166 ++++++++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 96 insertions(+), 70 deletions(-) > It seems I only have stylistic comments left for this, so FWIW Reviewed-by: Vegard Nossum Vegard