From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA083C433DF for ; Thu, 13 Aug 2020 17:19:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 81C9920774 for ; Thu, 13 Aug 2020 17:19:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=toxicpanda-com.20150623.gappssmtp.com header.i=@toxicpanda-com.20150623.gappssmtp.com header.b="YL0Vf/We" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726578AbgHMRTX (ORCPT ); Thu, 13 Aug 2020 13:19:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40446 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726334AbgHMRTX (ORCPT ); Thu, 13 Aug 2020 13:19:23 -0400 Received: from mail-qv1-xf43.google.com (mail-qv1-xf43.google.com [IPv6:2607:f8b0:4864:20::f43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BEB30C061757 for ; Thu, 13 Aug 2020 10:19:22 -0700 (PDT) Received: by mail-qv1-xf43.google.com with SMTP id y11so2988680qvl.4 for ; Thu, 13 Aug 2020 10:19:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=toxicpanda-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=gG3QhBSmvxEmkeggBu4YpeOeDbfqTTnXI4PDot46vj4=; b=YL0Vf/WekjB5AM1fC7TEY+E6+AVRqgLtZXfd25s9QHxsEHencNoK4s9J8XEWbUDCyW eipm4TQKkJVy8x2OpmcGo/sSjuWf6h1Ph9avHZp3SteAHGcWiS94x3uw7HzqpCzxgTIE D+FH60umOiEqnTrvVxKMqMaOflUpIvT5m6kMH+MLbYn1/puoRaw2wFJmWDCBJkinxKHj axsDdmko2LluqA+kzh89WeY+fF+/RFduhpJHvBSQ4nddTp2aOnUXLZ2wtToC5hYz14dz XUBVgKD4pu9wtN/UovuOeR8caAetpMTxc5RWl/MaEC9UKQv3mEKOai25E/h/okkbAbxs BnGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=gG3QhBSmvxEmkeggBu4YpeOeDbfqTTnXI4PDot46vj4=; b=e89ezVQESVlir0IM2TiMDEUQvk9xk2KVKeSErN+mnvERNs7RczZufXLrKDHz9Nr8Ao XjJSRB+YZjfEIHop8W23Mlj7Sn6w0g+RoyF9i236lSsss5Nu9ziVICBA3NcmQk+6jPpP T0IFf0OCIGzRXkJu0PTLRSiz4R5P7iwT2z/QK+TG8YmGMLdngq7qG0gTxF0LO+EhthP2 JehshVlpuc64y8SRbTR65p9rxxebwQVLdt9V61EDJJp3rrh1yz65EG5cf31Jz3QvP6KB ZqhrqhpIA0TpI+qBksuU9EKtHV9yOsQ+D6Tpd7cwqxZTkuJmu8uLgX/dZ09DyY8XYiIP PdZQ== X-Gm-Message-State: AOAM530gLrCVBNBUXitW9e0BVgME4hOPm8iMzI8yscik6FqsEGOA5YcT 1aQid2QwVroZ1gfXB3jrO6EJqyQ0HC6ApA== X-Google-Smtp-Source: ABdhPJwWp3EFYJ40IG50E8vXi2UTR2lQo5LyYbQiLWLmjIS0cYtoZwiOj9S4sgM/8CXPlK0MQJKrYg== X-Received: by 2002:a0c:f64a:: with SMTP id s10mr5666652qvm.196.1597339160681; Thu, 13 Aug 2020 10:19:20 -0700 (PDT) Received: from ?IPv6:2620:10d:c0a8:11d9::10a7? ([2620:10d:c091:480::1:fe9c]) by smtp.gmail.com with ESMTPSA id l13sm6749820qth.77.2020.08.13.10.19.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 13 Aug 2020 10:19:19 -0700 (PDT) Subject: Re: [PATCH][v2] proc: use vmalloc for our kernel buffer To: Al Viro , Christoph Hellwig Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, kernel-team@fb.com, willy@infradead.org References: <20200813145305.805730-1-josef@toxicpanda.com> <20200813153356.857625-1-josef@toxicpanda.com> <20200813153722.GA13844@lst.de> <974e469e-e73d-6c3e-9167-fad003f1dfb9@toxicpanda.com> <20200813154117.GA14149@lst.de> <20200813162002.GX1236603@ZenIV.linux.org.uk> From: Josef Bacik Message-ID: <9e4d3860-5829-df6f-aad4-44d07c62535b@toxicpanda.com> Date: Thu, 13 Aug 2020 13:19:18 -0400 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20200813162002.GX1236603@ZenIV.linux.org.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 8/13/20 12:20 PM, Al Viro wrote: > On Thu, Aug 13, 2020 at 05:41:17PM +0200, Christoph Hellwig wrote: >> On Thu, Aug 13, 2020 at 11:40:00AM -0400, Josef Bacik wrote: >>> On 8/13/20 11:37 AM, Christoph Hellwig wrote: >>>> On Thu, Aug 13, 2020 at 11:33:56AM -0400, Josef Bacik wrote: >>>>> Since >>>>> >>>>> sysctl: pass kernel pointers to ->proc_handler >>>>> >>>>> we have been pre-allocating a buffer to copy the data from the proc >>>>> handlers into, and then copying that to userspace. The problem is this >>>>> just blind kmalloc()'s the buffer size passed in from the read, which in >>>>> the case of our 'cat' binary was 64kib. Order-4 allocations are not >>>>> awesome, and since we can potentially allocate up to our maximum order, >>>>> use vmalloc for these buffers. >>>>> >>>>> Fixes: 32927393dc1c ("sysctl: pass kernel pointers to ->proc_handler") >>>>> Signed-off-by: Josef Bacik >>>>> --- >>>>> v1->v2: >>>>> - Make vmemdup_user_nul actually do the right thing...sorry about that. >>>>> >>>>> fs/proc/proc_sysctl.c | 6 +++--- >>>>> include/linux/string.h | 1 + >>>>> mm/util.c | 27 +++++++++++++++++++++++++++ >>>>> 3 files changed, 31 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >>>>> index 6c1166ccdaea..207ac6e6e028 100644 >>>>> --- a/fs/proc/proc_sysctl.c >>>>> +++ b/fs/proc/proc_sysctl.c >>>>> @@ -571,13 +571,13 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf, >>>>> goto out; >>>>> if (write) { >>>>> - kbuf = memdup_user_nul(ubuf, count); >>>>> + kbuf = vmemdup_user_nul(ubuf, count); >>>> >>>> Given that this can also do a kmalloc and thus needs to be paired >>>> with kvfree shouldn't it be kvmemdup_user_nul? >>>> >>> >>> There's an existing vmemdup_user that does kvmalloc, so I followed the >>> existing naming convention. Do you want me to change them both? Thanks, >> >> I personally would, and given that it only has a few users it might >> even be feasible. > > FWIW, how about following or combining that with "allocate count + 1 bytes on > the read side"? Allows some nice cleanups - e.g. > len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data); > if (len > left) > len = left; > memcpy(buffer, tmpbuf, len); > if ((left -= len) > 0) { > *((char *)buffer + len) = '\n'; > left--; > } > in sunrpc proc_dodebug() turns into > left -= snprintf(buffer, left, "0x%04x\n", > *(unsigned int *) table->data); > and that's not the only example. > We wouldn't even need the extra +1 part, since we're only copying in how much the user wants anyway, we could just go ahead and convert this to left -= snprintf(buffer, left, "0x%04x\n", *(unsigned int *) table->data); and be fine, right? Or am I misunderstanding what you're looking for? Thanks, Josef