Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934739AbdIYKoy (ORCPT ); Mon, 25 Sep 2017 06:44:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46466 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933162AbdIYKow (ORCPT ); Mon, 25 Sep 2017 06:44:52 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 447B08046F Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=mpatocka@redhat.com Date: Mon, 25 Sep 2017 06:44:49 -0400 (EDT) From: Mikulas Patocka X-X-Sender: mpatocka@file01.intranet.prod.int.rdu2.redhat.com To: Joe Lawrence cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, Alexander Viro , "Luis R. Rodriguez" , Kees Cook , Michael Kerrisk Subject: Re: [PATCH RFC 2/3] pipe: protect pipe_max_size access with a mutex In-Reply-To: <8b2369ef-a537-974e-725b-f55d49a646b6@redhat.com> Message-ID: References: <1504622676-2992-1-git-send-email-joe.lawrence@redhat.com> <1504622676-2992-3-git-send-email-joe.lawrence@redhat.com> <8b2369ef-a537-974e-725b-f55d49a646b6@redhat.com> User-Agent: Alpine 2.02 (LRH 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 25 Sep 2017 10:44:52 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5484 Lines: 177 On Tue, 19 Sep 2017, Joe Lawrence wrote: > On 09/14/2017 07:09 PM, Mikulas Patocka wrote: > > > > I think this mutex is too heavy - if multiple processes simultaneously > > create a pipe, the mutex would cause performance degradation. > > > > You can call do_proc_dointvec with a custom callback "conv" function > > that does the rounding of the pipe size value. > > Okay, that would consolidate the fiddling with pipe_max_size into a > single place / assignment. > > Implementation wise, are you suggesting something like the following > (work in progress). round_pipe_size() would still be used by > fs/pipe.c::pipe_set_size() so it would need to be visible to both source > files. The proc_do... function would be pipe-max-size specific as it's > basically: > > - proc_douintvec_minmax, but > - without the "max" check > - with an added PAGE_SIZE floor > - PAGE_SIZE increment > > and exported for pipe.c to call. > > Plumbing in the opposite direction (export do_proc_douintvec() and stick > the new conv implementation in fs/pipe.c) might be a little easier, but > seems like the same ick-factor in the end. > > Regards, > > -- Joe Yes. I think this is correct. Reviewed-by: Mikulas Patocka > -->8-- -->8-- -->8-- -->8-- > > diff --git a/fs/pipe.c b/fs/pipe.c > index 8cbc97d97753..4db3cd2d139c 100644 > --- a/fs/pipe.c > +++ b/fs/pipe.c > @@ -1019,7 +1019,7 @@ static int fifo_open(struct inode *inode, struct > file *filp) > * Currently we rely on the pipe array holding a power-of-2 number > * of pages. Returns 0 on error. > */ > -static inline unsigned int round_pipe_size(unsigned int size) > +unsigned int round_pipe_size(unsigned int size) > { > unsigned long nr_pages; > > @@ -1130,19 +1130,7 @@ static long pipe_set_size(struct pipe_inode_info > *pipe, unsigned long arg) > int pipe_proc_fn(struct ctl_table *table, int write, void __user *buf, > size_t *lenp, loff_t *ppos) > { > - unsigned int rounded_pipe_max_size; > - int ret; > - > - ret = proc_douintvec_minmax(table, write, buf, lenp, ppos); > - if (ret < 0 || !write) > - return ret; > - > - rounded_pipe_max_size = round_pipe_size(pipe_max_size); > - if (rounded_pipe_max_size == 0) > - return -EINVAL; > - > - pipe_max_size = rounded_pipe_max_size; > - return ret; > + return proc_dopipe_max_size(table, write, buf, lenp, ppos); > } > > /* > diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h > index e7497c9dde7f..485cf7a7aa8f 100644 > --- a/include/linux/pipe_fs_i.h > +++ b/include/linux/pipe_fs_i.h > @@ -190,5 +190,6 @@ static inline int pipe_buf_steal(struct > pipe_inode_info *pipe, > struct pipe_inode_info *get_pipe_info(struct file *file); > > int create_pipe_files(struct file **, int); > +unsigned int round_pipe_size(unsigned int size); > > #endif > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index 1d4dba490fb6..ba24ca72800c 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -50,6 +50,9 @@ extern int proc_dointvec_minmax(struct ctl_table *, int, > extern int proc_douintvec_minmax(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, > loff_t *ppos); > +extern int proc_dopipe_max_size(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, > + loff_t *ppos); > extern int proc_dointvec_jiffies(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int, > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index c976719bf37a..7a2913c5546e 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -67,6 +67,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -2631,6 +2632,47 @@ int proc_douintvec_minmax(struct ctl_table > *table, int write, > do_proc_douintvec_minmax_conv, ¶m); > } > > +struct do_proc_dopipe_max_size_conv_param { > + unsigned int *min; > +}; > + > +static int do_proc_dopipe_max_size_conv(unsigned long *lvalp, > + unsigned int *valp, > + int write, void *data) > +{ > + struct do_proc_dopipe_max_size_conv_param *param = data; > + > + if (write) { > + unsigned int val = round_pipe_size(*lvalp); > + > + if (val == 0) > + return -EINVAL; > + > + if (param->min && *param->min > val) > + return -ERANGE; > + > + if (*lvalp > UINT_MAX) > + return -EINVAL; > + > + *valp = val; > + } else { > + unsigned int val = *valp; > + *lvalp = (unsigned long) val; > + } > + > + return 0; > +} > + > +int proc_dopipe_max_size(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + struct do_proc_dopipe_max_size_conv_param param = { > + .min = (unsigned int *) table->extra1, > + }; > + return do_proc_douintvec(table, write, buffer, lenp, ppos, > + do_proc_dopipe_max_size_conv, ¶m); > +} > + > static void validate_coredump_safety(void) > { > #ifdef CONFIG_COREDUMP > @@ -3179,6 +3221,7 @@ int proc_doulongvec_ms_jiffies_minmax(struct > ctl_table *table, int write, > EXPORT_SYMBOL(proc_dointvec_jiffies); > EXPORT_SYMBOL(proc_dointvec_minmax); > EXPORT_SYMBOL_GPL(proc_douintvec_minmax); > +EXPORT_SYMBOL_GPL(proc_dopipe_max_size); > EXPORT_SYMBOL(proc_dointvec_userhz_jiffies); > EXPORT_SYMBOL(proc_dointvec_ms_jiffies); > EXPORT_SYMBOL(proc_dostring); > -- > 1.8.3.1 >