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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A706C43334 for ; Fri, 1 Jul 2022 06:10:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233375AbiGAGKy (ORCPT ); Fri, 1 Jul 2022 02:10:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43712 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234693AbiGAGKu (ORCPT ); Fri, 1 Jul 2022 02:10:50 -0400 Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E08C61DA62; Thu, 30 Jun 2022 23:10:49 -0700 (PDT) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 4F5945C0061; Fri, 1 Jul 2022 02:10:49 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Fri, 01 Jul 2022 02:10:49 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=themaw.net; h=cc :cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1656655849; x= 1656742249; bh=k1W5DMoXurzbQJ/ZkPRxE74KndqCvp2BPtDKdSkedwI=; b=X /VV+8trPBbb+vBv+SicCHaboOyjXmjEer0xpn0FMOPvwPjLaLAe7b3yZEJvZY8rm q6yHDf/Tra+AyoTbYQlDtgu1/48T2oBhPbd3zlIroHVAYxj6GzzyN0UtT0lB+XO/ +mYUBPlUtEHmKWfHIHRYt4IXF4YcJWAEnfZXU0p6eAVuzTN/L1EXCU3hw1n+HzaP eO/Aod+GrI4R3qMU8O899GKxf3DbLrTJpERXsx806gF4u+IQSjtadPrl3J9rEWlS UCAVxHfo3ZmNetc6vrGrjKx4sz3HioKpmIHnzqp4Jx86NgWYCxrSi8t6Ps7abaVc WLn3tDbxo8Z5liisPFjXA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t=1656655849; x= 1656742249; bh=k1W5DMoXurzbQJ/ZkPRxE74KndqCvp2BPtDKdSkedwI=; b=t FbIizNm9mYjIE7LSip8RT434fbWbtZE9xivCXQAXIGgNv7uMpxPUqAXg1aiM40M3 Fd89Sfrl2RxTJ5sNxtC6VF8B8gjzwJegH1nGLZWOu4kw+NsZVu1ZCCo/QBpfdnrK DG7V4FDoioQoT+DD3nF4dSYq5mejuAoYt4JPM526jUz0ntF9+Y7yd69hc64YLd+L Dak7utSihyCSfaaqmP9lG54tKrVJbvw9aiAyem0mnZMaRdRMTQCpvAymuV4JcTAt jdxEFq6JOjhg1j8T/tFpdXM14OYg+JFYKpKQqRgepzT9qP8AymOqGq3HhxzGf3cb MmxSfpSKzHhik0Ms2tGAw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrudehvddguddtkecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefkffggfgfuvfevfhfhjggtgfesthekredttdefjeenucfhrhhomhepkfgr nhcumfgvnhhtuceorhgrvhgvnhesthhhvghmrgifrdhnvghtqeenucggtffrrghtthgvrh hnpeegvdetvedvfeeivdeuueejgeetvdehlefhheethfekgfejueffgeeugfekudfhjeen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehrrghvvg hnsehthhgvmhgrfidrnhgvth X-ME-Proxy: Feedback-ID: i31e841b0:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 1 Jul 2022 02:10:46 -0400 (EDT) Message-ID: <22f9fbb7-a557-f372-7ede-92f0af338bd1@themaw.net> Date: Fri, 1 Jul 2022 14:10:41 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [REPOST PATCH] nfs: fix port value parsing Content-Language: en-US To: Trond Myklebust , "Anna.Schumaker@Netapp.com" Cc: "linux-nfs@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" , "SteveD@redhat.com" , "bcodding@redhat.com" , "dhowells@redhat.com" References: <165637590710.37553.7481596265813355098.stgit@donald.themaw.net> <891563475afc32c49fab757b8b56ecdc45b30641.camel@hammerspace.com> From: Ian Kent In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On 30/6/22 07:57, Trond Myklebust wrote: > On Thu, 2022-06-30 at 07:33 +0800, Ian Kent wrote: >> On 29/6/22 23:33, Trond Myklebust wrote: >>> On Wed, 2022-06-29 at 09:02 +0800, Ian Kent wrote: >>>> On 28/6/22 22:34, Trond Myklebust wrote: >>>>> On Tue, 2022-06-28 at 08:25 +0800, Ian Kent wrote: >>>>>> The valid values of nfs options port and mountport are 0 to >>>>>> USHRT_MAX. >>>>>> >>>>>> The fs parser will return a fail for port values that are >>>>>> negative >>>>>> and the sloppy option handling then returns success. >>>>>> >>>>>> But the sloppy option handling is meant to return success for >>>>>> invalid >>>>>> options not valid options with invalid values. >>>>>> >>>>>> Parsing these values as s32 rather than u32 prevents the >>>>>> parser >>>>>> from >>>>>> returning a parse fail allowing the later USHRT_MAX option >>>>>> check >>>>>> to >>>>>> correctly return a fail in this case. The result check could >>>>>> be >>>>>> changed >>>>>> to use the int_32 union variant as well but leaving it as a >>>>>> uint_32 >>>>>> check avoids using two logical compares instead of one. >>>>>> >>>>>> Signed-off-by: Ian Kent >>>>>> --- >>>>>>    fs/nfs/fs_context.c |    4 ++-- >>>>>>    1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c >>>>>> index 9a16897e8dc6..f4da1d2be616 100644 >>>>>> --- a/fs/nfs/fs_context.c >>>>>> +++ b/fs/nfs/fs_context.c >>>>>> @@ -156,14 +156,14 @@ static const struct fs_parameter_spec >>>>>> nfs_fs_parameters[] = { >>>>>>           fsparam_u32   ("minorversion",  Opt_minorversion), >>>>>>           fsparam_string("mountaddr",     Opt_mountaddr), >>>>>>           fsparam_string("mounthost",     Opt_mounthost), >>>>>> -       fsparam_u32   ("mountport",     Opt_mountport), >>>>>> +       fsparam_s32   ("mountport",     Opt_mountport), >>>>>>           fsparam_string("mountproto",    Opt_mountproto), >>>>>>           fsparam_u32   ("mountvers",     Opt_mountvers), >>>>>>           fsparam_u32   ("namlen",        Opt_namelen), >>>>>>           fsparam_u32   ("nconnect",      Opt_nconnect), >>>>>>           fsparam_u32   ("max_connect",   Opt_max_connect), >>>>>>           fsparam_string("nfsvers",       Opt_vers), >>>>>> -       fsparam_u32   ("port",          Opt_port), >>>>>> +       fsparam_s32   ("port",          Opt_port), >>>>>>           fsparam_flag_no("posix",        Opt_posix), >>>>>>           fsparam_string("proto",         Opt_proto), >>>>>>           fsparam_flag_no("rdirplus",     Opt_rdirplus), >>>>>> >>>>>> >>>>> Why don't we just check for the ENOPARAM return value from >>>>> fs_parse()? >>>> In this case I think the return will be EINVAL. >>> My point is that 'sloppy' is only supposed to work to suppress the >>> error in the case where an option is not found by the parser. That >>> corresponds to the error ENOPARAM. >> Well, yes, and that's why ENOPARAM isn't returned and shouldn't be. >> >> And if the sloppy option is given it doesn't get to check the value >> >> of the option, it just returns success which isn't right. >> >> >>>> I think that's a bit to general for this case. >>>> >>>> This seemed like the most sensible way to fix it. >>>> >>> Your patch works around just one symptom of the problem instead of >>> addressing the root cause. >>> >> Ok, how do you recommend I fix this? >> > Maybe I'm missing something, but why not this? > > 8<-------------------------------- > diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c > index 9a16897e8dc6..8f1f9b4af89d 100644 > --- a/fs/nfs/fs_context.c > +++ b/fs/nfs/fs_context.c > @@ -484,7 +484,7 @@ static int nfs_fs_context_parse_param(struct > fs_context *fc, > > opt = fs_parse(fc, nfs_fs_parameters, param, &result); > if (opt < 0) > - return ctx->sloppy ? 1 : opt; > + return (opt == -ENOPARAM && ctx->sloppy) ? 1 : opt; > > if (fc->security) > ctx->has_sec_mnt_opts = 1; > I tested this with the autofs connectathon tests I use which has lots of success and fail cases. As expected there were no surprises, the tests worked fine and gave the expected results. I'll send an updated patch, is a "Suggested-by" attribution sufficient or would you like something different? Ian