From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f67.google.com ([209.85.213.67]:45096 "EHLO mail-vk0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752128AbeAIW1M (ORCPT ); Tue, 9 Jan 2018 17:27:12 -0500 Received: by mail-vk0-f67.google.com with SMTP id o16so10392346vke.12 for ; Tue, 09 Jan 2018 14:27:11 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20180108053542.6472-7-ebiggers3@gmail.com> References: <20180108053542.6472-1-ebiggers3@gmail.com> <20180108053542.6472-7-ebiggers3@gmail.com> From: Kees Cook Date: Tue, 9 Jan 2018 14:27:10 -0800 Message-ID: Subject: Re: [PATCH 6/7] pipe: simplify round_pipe_size() To: Eric Biggers Cc: "linux-fsdevel@vger.kernel.org" , Alexander Viro , Joe Lawrence , Michael Kerrisk , Willy Tarreau , Mikulas Patocka , "Luis R . Rodriguez" , LKML , Eric Biggers Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers wrote: > From: Eric Biggers > > round_pipe_size() calculates the number of pages the requested size > corresponds to, then rounds the page count up to the next power of 2. > > However, it also rounds everything < PAGE_SIZE up to PAGE_SIZE. > Therefore, there's no need to actually translate the size into a page > count; we just need to round the size up to the next power of 2. > > We do need to verify that bit 31 isn't set, since on 32-bit systems > roundup_pow_of_two() would be undefined in that case. But that can just > be combined with the UINT_MAX check which we need anyway now. > > Finally, also remove the check for '!nr_pages' in pipe_set_size(), since > round_pipe_size() always returns either 0 or a multiple of PAGE_SIZE. > > Signed-off-by: Eric Biggers > --- > fs/pipe.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/fs/pipe.c b/fs/pipe.c > index f1ee1e599495..774cafd947dc 100644 > --- a/fs/pipe.c > +++ b/fs/pipe.c > @@ -1022,20 +1022,14 @@ const struct file_operations pipefifo_fops = { > */ > unsigned int round_pipe_size(unsigned long size) > { > - unsigned long nr_pages; > - > - if (size > UINT_MAX) > + if (size > (1U << 31)) > return 0; > > /* Minimum pipe size, as required by POSIX */ > if (size < PAGE_SIZE) > - size = PAGE_SIZE; > - > - nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; > - if (nr_pages == 0) > - return 0; > + return PAGE_SIZE; > > - return roundup_pow_of_two(nr_pages) << PAGE_SHIFT; > + return roundup_pow_of_two(size); > } > > /* Above looks good. > @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) > return -EINVAL; > nr_pages = size >> PAGE_SHIFT; > > - if (!nr_pages) > - return -EINVAL; > - I would just leave this hunk anyway: it's defensive for any future changes. Maybe add a comment describing why it's currently redundant? -Kees > /* > * If trying to increase the pipe capacity, check that an > * unprivileged user is not trying to exceed various limits > -- > 2.15.1 > -- Kees Cook Pixel Security