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=-10.4 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 E23C0C433E0 for ; Mon, 18 Jan 2021 11:07:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AC8B9221F8 for ; Mon, 18 Jan 2021 11:07:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389262AbhARLHQ (ORCPT ); Mon, 18 Jan 2021 06:07:16 -0500 Received: from mx2.suse.de ([195.135.220.15]:48000 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389381AbhARKvJ (ORCPT ); Mon, 18 Jan 2021 05:51:09 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 92837AB7F; Mon, 18 Jan 2021 10:50:25 +0000 (UTC) To: Xiaoming Ni , Michal Hocko , Andrew Morton , rdunlap@infradead.org, hkallweit1@gmail.com Cc: linux-kernel@vger.kernel.org, mcgrof@kernel.org, keescook@chromium.org, yzaikin@google.com, adobriyan@gmail.com, linux-fsdevel@vger.kernel.org, andy.shevchenko@gmail.com, wangle6@huawei.com References: <20210112033155.91502-1-nixiaoming@huawei.com> <20210111203340.98dd3c8fa675b709bcf6d49e@linux-foundation.org> <89d1369e-f0a8-66f2-c0ea-3aac3a55e2c1@huawei.com> <20210111222845.67ceb4e3c7f64f267756e4e8@linux-foundation.org> <20210112072406.GF22493@dhcp22.suse.cz> From: Vlastimil Babka Subject: Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. Message-ID: <278b5b2e-c256-730a-8bfc-1e19b20785a8@suse.cz> Date: Mon, 18 Jan 2021 11:50:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/17/21 3:59 AM, Xiaoming Ni wrote: > On 2021/1/12 19:42, Vlastimil Babka wrote: >> On 1/12/21 8:24 AM, Michal Hocko wrote: >>>>>> >>>>>> If we're going to do a separate "patch: make process_sysctl_arg() >>>>>> return an errno instead of 0" then fine, we can discuss that.  But it's >>>>>> conceptually a different work from fixing this situation. >>>>>> . >>>>>> >>>>> However, are the logs generated by process_sysctl_arg() clearer and more >>>>> accurate than parse_args()? Should the logs generated by >>>>> process_sysctl_arg() be deleted? >>>> >>>> I think the individual logs are very useful and should be retained. >>> >>> Yes, other sysfs specific error messages are likely useful. I just fail >>> to see why a missing value should be handled here when there is an >>> existing handling in the caller. Not sure whether a complete shadow >>> reporting in process_sysctl_arg is a deliberate decision or not. >>> Vlastimil? >> >> Yes, it's a way to have more useful sysctl-specific reports than the generic >> ones. And I think I was inspired by some other existing code, but don't remember >> exactly. The options are: >> >> 1) the current sysctl-specific reports, return 0 as the values are only consumed >> 2) be silent and return error, invent new error codes to have generic report be >> more useful for sysctl, but inevitably lose some nuances anyway >> 3) a mix where 2) is used for situations where generic report is sufficient >> enough, 1) where not >> >> Patch v2 went with option 1), v3 with option 3). I think it's down to >> preferences. I would personally go with v2 and message similar to the existing >> ones, i.e.: >> >> "Failed to set sysctl parameter '%s': no value given\n" >> >> Also we seem to be silently doing nothing when strlen(val) == 0, i.e. >> "hung_task_panic=" was passed. Worth reporting the same error. >> >> But v3 is fine with me as well. The generic error message works. We could just >> add "if (!len) return -EINVAL" below the strlen() call. >> >> Also please Cc: stable. >> >>> Anyway one way or the other, all I care about is to have a reporting in >>> place because this shouldn't be a silent failure. >>> > > > The current v2 is already in the linux-next branch and throws a new error: > https://lore.kernel.org/lkml/cb54e349-7147-0a1f-a349-1e16ba603fce@infradead.org/ > > This bug has been mentioned in the previous discussion and has been fixed in the > current v3 patch. > https://lore.kernel.org/linux-fsdevel/202101111149.20A58E1@keescook/ > > What am I supposed to do now? >     - Resend V3? IMHO this. But also please handle also len == 0 like below. And add Cc: >     - Rewrite a new fix patch based on the current code of linux-next. AFAICS Andrew dropped the v2 already. Thanks. diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 317899222d7f..f424010d1a60 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1770,6 +1770,12 @@ static int process_sysctl_arg(char *param, char *val, return 0; } + if (!val) + return -EINVAL; + len = strlen(val); + if (!len) + return -EINVAL; + /* * To set sysctl options, we use a temporary mount of proc, look up the * respective sys/ file and write to it. To avoid mounting it when no @@ -1811,7 +1817,6 @@ static int process_sysctl_arg(char *param, char *val, file, param, val); goto out; } - len = strlen(val); wret = kernel_write(file, val, len, &pos); if (wret < 0) { err = wret;