Received-SPF: pass (google.com: domain of linux-nfs+bounces-3834-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 14 Jun 2024 09:24:22 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Jakub Kicinski <kuba@kernel.org>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] nfsd: fix error handling bug in
 nfsd_nl_pool_mode_get_doit
Message-ID: <ZmxEhlVWsj5nlsLa@tissot.1015granger.net>
References: <20240614-nfsd-next-v1-1-d360eea79d0b@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240614-nfsd-next-v1-1-d360eea79d0b@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?AbScgKGq6IIMqKqG0gdOPIxzdpveaJrMO9Iv0eMbNJ/kkxgaANXcfGSvlGrS?=
 =?us-ascii?Q?lqD+ZzlAL2QTBURNOE+1OkN1Xmwdxr667n0F3NYYqrwqRPONiMgHRnnjbjI6?=
 =?us-ascii?Q?5ueWjB6T9nvKu60HBVi6fokDn5n1dRoWptC9LToT0lQq5NvAVQlQF/127Jff?=
 =?us-ascii?Q?2sDZjBrdatNCUfoyjaZs3RLBvD1cm88E9CrYuBSpERHzh37Bef0QizOxGqtA?=
 =?us-ascii?Q?4f2jIR9F/cv777oQWItRbD+vVlMinkz/QhYoe690X/94YaCpHMD6FKaufRRy?=
 =?us-ascii?Q?Et45+v/402KQ+LnA7WzA5et2BUWeBuV9RMl5NxTYFnpxqSBL0jWybBW+WsbL?=
 =?us-ascii?Q?DJSeG9oMKPv0RDuGjGben8LFUY5jM6m5D2o574jf4fn1okYT0zXHoM1NR7jV?=
 =?us-ascii?Q?jhFok724uwvj1stT833rACkseB7WEyuG5kSWrUOs4PHSa4XKg4zbW+4xLrKV?=
 =?us-ascii?Q?FLJxJch5bK6lVsFpGQE96hMNmCsh7kB0Z2AyyFbbxOiCnwx/nVnXTe7lZDZL?=
 =?us-ascii?Q?VSiSXSnFHMkxpOq5BDTmaqTtt6uvJhX9VVfqjjHimclHDwIRNw8SqN+tlaHv?=
 =?us-ascii?Q?fWUmRSgq8vfbN90OwCaUH5Wng+5njFAInu0jgoXll7xoUu5hob0wCxxdvTjX?=
 =?us-ascii?Q?YWCCyPViWhXP43ArjqhqfhIW5CqhbnjIKwTiQzix1PQ89cXvLyIpbOcpRIS/?=
 =?us-ascii?Q?gLC/mGzE/ibqEedz/UgAjHPBevZNUmDQlGDwKaUFSWX2sZClLydHPfu0Wy80?=
 =?us-ascii?Q?wwe4m4rN02tLwe/A/+vW3BILCcf9R5W1pC3WmLjhurwG9RT6ZfIIjoCA+FBf?=
 =?us-ascii?Q?alTdumJpX/E+AwG0oaRreawx8mXtSkbSPiTWbXOe5ihSU5bTVL3ETTawpPVf?=
 =?us-ascii?Q?XURRtATRB+4bQrX4aGGpixCvlBPgLOy0CllmIEYcMbxjlxLfqxtp4mvA62Xe?=
 =?us-ascii?Q?UTi++8Lj4Gof2p0lgZXDVeQgww86Ojx81Q3Bo1zgo+7BSxYIdwgJSEcOcXbm?=
 =?us-ascii?Q?obnq3A9CQtybxRbdHQ/IdY7kEQsF3Y0pmMVx90/VLA6e35/oQgALmEqjG8YX?=
 =?us-ascii?Q?WV8Fv6VRlprXbe9HjL/5xZcvDB+06o5us/LLDY49d/BtfR36zPpGM69Gm6QQ?=
 =?us-ascii?Q?XkTZz39pnDQ/O+DmABZdpIUvFg1ECjj7WZ+AUBEXQ4aBEyXaqasJTlKRvfz5?=
 =?us-ascii?Q?5zGWimiMU9naAdRxMN0ZfkMG/nnVm3+RXyyg7ATjzhZN1Wc8vOSnXoD/HEj9?=
 =?us-ascii?Q?+DpQGoP5Jq/hFMFyNvudPSX72+Bi2AyXbfe0k6j1YY2gCy98LI/zUqB+HtEh?=
 =?us-ascii?Q?mVk3yGmt4Y4R1gJ+j5IxEbdu08pDU3AcSRkw6Ba3fWRDgq/4Lj4YRDOtpeYk?=
 =?us-ascii?Q?Vf2oA1wOcIqaelHy8PIh8N0qQ9QvnNWloh+MWTGBVxOg/zmcJnFZlIS7wSvH?=
 =?us-ascii?Q?eXNRtMZCFtdvxzukcIP+tkIRICfiaoPGERbUnvIKNchZJXTIhA04FmFpZbpN?=
 =?us-ascii?Q?WEnaISqGnIvVLXMnsYJLW55tRykEi1hCgT/RlGtZpisw++iOXqVDIVqV2YSf?=
 =?us-ascii?Q?cR61wZUkRU0gXGMQ1vPQszcB0zKQocYRZ04tJkHOgmXVr9eGpWmAPb7UuTsz?=
 =?us-ascii?Q?4A=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	AC0GITauxgZc5GP4fG16u0z3vo6eeo85kp/v6F/lSsOkh1bza+6fiFuSOKHJtmg+j4bRIf/xUBYnBz6J2aSS9G9hRq65LDeQWBW9JpbN94wdrKgG2tX8kfe5gb13+JawdQOqGfzokNaxPqAvB9TqadNsKWL0+rjeDAqcI31TJaNfm3MCdEzVdy/jxrF0Fp661XLuCyPA9dgz0OGUu8AwT60sUWzOF0knqtNDjUaJL635rRKEEm0OB05UIlPtgmBkrht7it3UqXTX4RYKk0vjZeAUrGSQBGx84/Gp/CrqJoHZLZgIURyQhpXHAkN2knU1hNtvim0EZvzcbZ95wOm+eaCxMDvnG+CRxa70xQN6SiYb1w/z2tYje+M2GIUMMCLZmE5lsSh8SKxhUv1HPhlprBcrYskA/Ms9B04glvAdeajptRJX/9rdrfmTGZx+fmMHwVDPyVxBmlSVz3r/B3mSrNKahatAVAYHG973A4poP5FXhVTLr4nvuP+vVibou1t8kTqVUbegh3XJsY6oWQpECDVyPqXPgaD33KZn6xeDqa5VpEAVfMW6VCoYLZmxxbyxtj0haoJUjM9K9+W46QyS/8IvlPPVZfeLfKj7Pn0ef8Q=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 687a8fc5-9323-458e-e786-08dc8c754f0c
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 13:24:24.6702
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2cbJStY10F7zpnJm3XoteyqZ27qylyZwz3rzSjy39v7nJa8Zzq6sAKsGqQkzvyvf3pbQvl+He4G62xNr6deaig==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB7056
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-14_10,2024-06-14_03,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0
 phishscore=0 bulkscore=0 malwarescore=0 spamscore=0 mlxscore=0
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406140090
X-Proofpoint-GUID: ZjbcIC58_A7ffiP8HyIHfLzwd09Z0jwF
X-Proofpoint-ORIG-GUID: ZjbcIC58_A7ffiP8HyIHfLzwd09Z0jwF

On Fri, Jun 14, 2024 at 08:59:10AM -0400, Jeff Layton wrote:
> As Jakub pointed out, I meant to use a bitwise or after the call to
> nla_put_string. Also move the sunrpc_get_pool_mode call up in the
> function, which simplifies the error handling a bit.
> 
> Cc: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Technically, I think the logical or works just as well as a bitwise or
> here, but this is cleaner and maybe slightly more efficient. Chuck,
> it's probably best to squash this into 84a570328ee.

Done, and pushed to nfsd-next.


> ---
>  fs/nfsd/nfsctl.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 187e9be77b78..e5d2cc74ef77 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -2214,6 +2214,9 @@ int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info)
>  	void *hdr;
>  	int err;
>  
> +	if (sunrpc_get_pool_mode(buf, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
> +		return -ERANGE;
> +
>  	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!skb)
>  		return -ENOMEM;
> @@ -2223,11 +2226,7 @@ int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (!hdr)
>  		goto err_free_msg;
>  
> -	err = -ERANGE;
> -	if (sunrpc_get_pool_mode(buf, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
> -		goto err_free_msg;
> -
> -	err = nla_put_string(skb, NFSD_A_POOL_MODE_MODE, buf) ||
> +	err = nla_put_string(skb, NFSD_A_POOL_MODE_MODE, buf) |
>  	      nla_put_u32(skb, NFSD_A_POOL_MODE_NPOOLS, nfsd_nrpools(net));
>  	if (err)
>  		goto err_free_msg;
> 
> ---
> base-commit: 84a570328eefd4df2e201deb5d43d152e0aca55a
> change-id: 20240614-nfsd-next-d5f6bbdf027d
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Chuck Lever