From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751374AbdBJFuS (ORCPT ); Fri, 10 Feb 2017 00:50:18 -0500 Received: from mail.kernel.org ([198.145.29.136]:49934 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750940AbdBJFuR (ORCPT ); Fri, 10 Feb 2017 00:50:17 -0500 Date: Fri, 10 Feb 2017 14:50:06 +0900 From: Masami Hiramatsu To: Steven Rostedt , Ingo Molnar Cc: LKML , Srikar Dronamraju , Namhyung Kim , Masami Hiramatsu , Andrew Morton Subject: Re: [RFC][PATCH] tracing: Have traceprobe_probes_write() not access userspace unnecessarily Message-Id: <20170210145006.b387236e4b6813a09d55162b@kernel.org> In-Reply-To: <20170209180458.5c829ab2@gandalf.local.home> References: <20170209180458.5c829ab2@gandalf.local.home> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 9 Feb 2017 18:04:58 -0500 Steven Rostedt wrote: > > The code in traceprobe_probes_write() reads up to 4096 bytes from userpace > for each line. If userspace passes in several lines to execute, the code > will do a large read for each line, even though, it is highly likely that > the first read from userspace received all of the lines at one. > > I changed the logic to do a single read from userspace, and to only read > from userspace again if not all of the read from userspace made it in. > > I tested this by adding printk()s and writing files that would test -1, ==, > and +1 the buffer size, to make sure that there's no overflows and that if a > single line is written with +1 the buffer size, that it fails properly. > Thanks Steve! Acked-by: Masami Hiramatsu BTW, this can conflict with my previous patch. https://lkml.org/lkml/2017/2/6/1048 https://lkml.org/lkml/2017/2/7/203 I'll update this. Ingo, Can I send these patch to Steve? Thank you, > Signed-off-by: Steven Rostedt (VMware) > --- > kernel/trace/trace_probe.c | 48 ++++++++++++++++++++++++++++------------------ > 1 file changed, 29 insertions(+), 19 deletions(-) > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > index 8c0553d..2a06f1f 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -647,7 +647,7 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer, > size_t count, loff_t *ppos, > int (*createfn)(int, char **)) > { > - char *kbuf, *tmp; > + char *kbuf, *buf, *tmp; > int ret = 0; > size_t done = 0; > size_t size; > @@ -667,27 +667,37 @@ ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer, > goto out; > } > kbuf[size] = '\0'; > - tmp = strchr(kbuf, '\n'); > + buf = kbuf; > + do { > + tmp = strchr(buf, '\n'); > + if (tmp) { > + *tmp = '\0'; > + size = tmp - buf + 1; > + } else { > + size = strlen(buf); > + if (done + size < count) { > + if (buf != kbuf) > + break; > + pr_warn("Line length is too long: Should be less than %d\n", > + WRITE_BUFSIZE); > + ret = -EINVAL; > + goto out; > + } > + } > + done += size; > > - if (tmp) { > - *tmp = '\0'; > - size = tmp - kbuf + 1; > - } else if (done + size < count) { > - pr_warn("Line length is too long: Should be less than %d\n", > - WRITE_BUFSIZE); > - ret = -EINVAL; > - goto out; > - } > - done += size; > - /* Remove comments */ > - tmp = strchr(kbuf, '#'); > + /* Remove comments */ > + tmp = strchr(buf, '#'); > > - if (tmp) > - *tmp = '\0'; > + if (tmp) > + *tmp = '\0'; > > - ret = traceprobe_command(kbuf, createfn); > - if (ret) > - goto out; > + ret = traceprobe_command(buf, createfn); > + if (ret) > + goto out; > + buf += size; > + > + } while (done < count); > } > ret = done; > > -- > 2.9.3 > -- Masami Hiramatsu