Received-SPF: pass (google.com: domain of linux-nfs+bounces-2890-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 17 Apr 2024 14:56:07 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>,
        Anna Schumaker <anna.schumaker@netapp.com>, linux-nfs@vger.kernel.org,
        Dan Carpenter <dan.carpenter@linaro.org>
Subject: Re: [PATCH] NFSv4: Fixup smatch warning for ambiguous return
Message-ID: <ZiAbR5oTqMsBw/T5@tissot.1015granger.net>
References: <b1577a24c58a9b0605a4540c8be5c411a07cb04c.1713379239.git.bcodding@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b1577a24c58a9b0605a4540c8be5c411a07cb04c.1713379239.git.bcodding@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?74IcqHvZDaZwVXi5r45tMvRUdPku+DIl7J1KFzJ7h5kehCQ/TdKrfy6CTCtE?=
 =?us-ascii?Q?lkHobmIk3lRLnC0xIXqihtcPtA/rHj0lKdXkuc6327Qrdws40mViysdNUGVi?=
 =?us-ascii?Q?zCofZOym0z4gEg6hJe1VRnjvgeO3nVghjLJ9J9i3/Yn0YdKHTvYjvIzoAM98?=
 =?us-ascii?Q?T428Jk6qGYQr6/WyAoAAjytOXrQRZUmKjgwzWYDrB3eSByIZYTouqLQ22Pk2?=
 =?us-ascii?Q?DMmc7whSSWixeMurB9EtLJfHopCFwV8kLiy1PxgWHsefB2/9ZWO2KwuYNstF?=
 =?us-ascii?Q?N26PE9u8zUqk8LPJ29hbUuR02REOjpQJkYGD3OzW36VX3eDlD9xfRGRW+fzA?=
 =?us-ascii?Q?9IJuOtdk/R4uPGlVv7OAhqI/06mVzN3bqFJJQ08Yu+dKJX4y+AMiIlYvkhm8?=
 =?us-ascii?Q?odZY54GziyLrGRcpQCv6/ss2P0B7oCU0D0MloSeTiRvF6RuwJEE+bQXdxZZu?=
 =?us-ascii?Q?09cJQP61lohcsYAkW4a8kYgaJqAzOeJczAV2LoGbm1cpWhTy5hRiwMbyS3V0?=
 =?us-ascii?Q?Fgpp7cVaVAyuuWJ/57ULn21qEYASDB5OdQp9pLuprQR2er3k9bTgfcv3+OPC?=
 =?us-ascii?Q?bKrbqDnOmbKOGELAFEZGxt68mRxLHRVc/kKXbLLV1YXVJxhPDV7siBU2fFGV?=
 =?us-ascii?Q?09aSPzn5C7BFl4I+ZLSOJ3AuEKEVqaL2uo3GeEUruYb3h3s9PaF9ydDnVTS3?=
 =?us-ascii?Q?tarP/Pl4dG5uzoT3s0V0W+vwS+C1GKi9IH+9ar5Mfz9fY/LgKaWvV5OxPtT7?=
 =?us-ascii?Q?FbPsS2fySKGpAzeJb7RH8+lGNZpjtydB67rQn6+czHmhdrT+tlbdueikgUTL?=
 =?us-ascii?Q?sOnXbySZ/MzSzyUbxUK4JoSxXEQ/CfcVHQ9cHhBKG5PZt3PmY5FF3EG7Nfu5?=
 =?us-ascii?Q?svuW2r4FHZ+OyofCD2TudMxmxvgVZCzelG9Eq9gnXIsEu9MtW68gfJLTdAvX?=
 =?us-ascii?Q?NBwiMzoYXV+QCTP2xKw7nRaGl55cqHAXh3lETKCqZGhOcd8MT7A8pupT091/?=
 =?us-ascii?Q?FhO6bLwIqo2QmhsznVn/1Ri1xD2oAgGCS/Qy6WoqNB7WVmdOapJTq8KBQmbt?=
 =?us-ascii?Q?mhPbylDxNTEePU9vqs+/Fw49lGnvw4zHaLRfnKJ69hHD+yKuY4PLHA0FyvWu?=
 =?us-ascii?Q?AYx8ZagJW1COOYMnz1kEb7iqze/+VwIsAu46M58kLy9/D9vMM96hv8jjIBP+?=
 =?us-ascii?Q?lqSH2V9QRG9/CaJ6hI7UKUYKaD4wkR/eDuCCHwCDod9xF8249YFQBU7qNxjI?=
 =?us-ascii?Q?X624U5mcfUZ6hXgDH8ULK7Hqwqr3Lp2MimQD0yeXgqOAK/MXwCbajvhKelCn?=
 =?us-ascii?Q?Nlbhw4CaOmdZAvpPmUyEYYn8fXN/2aL5ETW5sLzwECZ4ooRtkLN3gXUX09Rl?=
 =?us-ascii?Q?HoXzYz7G0+a2PHmBWYfP+GTx5r/cat5QQC22W//yyJ2Herb4J4da/SHDGJX5?=
 =?us-ascii?Q?LS+U7250itKjOAupAWFghvOQUI6SAmPjtIJqBLTSh5PogbBISuV+mCcQx4sq?=
 =?us-ascii?Q?Q5daDJ9erl64WYDQks3p2ZhQLUnSXX69GVh7sZCSxobZ2QJepLfYx8Rortqe?=
 =?us-ascii?Q?ykTbfiHLGHjM9y5njgu579ihIK1u1KxdGss65Y+05VEoHRXuuF/i/kGkmnVu?=
 =?us-ascii?Q?DA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	uDw7xV6Gl7DfKJt/hCYD3cAQL94eNdxT9pYq/r0F3K9CgsFePqLiSn1WcvQrTY3+qZVS5m6F2q7OHVfXbT+VfDRTsOOjPHIuB1sXaEheda3sz2jbT+EelNd1NQWbJ8/h1yoNnzUxydgiOK1x9Y0urkYiWEAm9vT2RPYhjxDhqZOgOMJZqemLJqGSMYhJTDzKuZxh9mGilLYjjZNEV96tOeToq6T6SZs3ZhSMIBsfuLdK7uOH3d6WgOtyA1dDdN9IIPK/ShVHbdcsdIVwouYxgCR8e/0XyK+Da2jtdTmj9Ypv4QFkp5S05BDQaRqH5yQ9GfDE8Xm6ZNnzFCYlmVM/IUFmoFBmeD1pmoSRgW76CyBkNdjK0Kw5ylLmVGFT5QZL/hUZMvyYmLI8tkVDdiCDmx6iUkUmcu4kKsweWG3M6uAFhB3Ls55yq5xKbd2sUcqM8yORCMeHg3iD6mx4xDNwqtf3SI8ZOoi+35Gxzla2QAozurT5rwkBPaosQVwbSrtntSOrQD/jzckbrcTpVofQYbsw6pTFgKQoJ9EpBH/UkdwkpjClt1tTwgXO4K3zbTb7SQZ6vfuKyLGfOuXIbofLYI7QLh0KF6oxwG8m56uHIUM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 43499a00-abbc-4ae2-0917-08dc5f100bb1
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2024 18:56:10.1244
 (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: skNb1t3/OPaU1/Q7/McUNx9WF3uFtBFpTbH7b/rdTvhyXo9nRtcTbo+LYTBV20V9jM/V4/w9hLhQidb8vLaytw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB6037
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-17_16,2024-04-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0
 mlxlogscore=999 phishscore=0 mlxscore=0 malwarescore=0 bulkscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404170135
X-Proofpoint-GUID: b59ian7QXyUJ3j6NV_z-C4hf3m6GXkyv
X-Proofpoint-ORIG-GUID: b59ian7QXyUJ3j6NV_z-C4hf3m6GXkyv

On Wed, Apr 17, 2024 at 02:49:29PM -0400, Benjamin Coddington wrote:
> Dan Carpenter reports smatch warning for nfs4_try_migration() when a memory
> allocation failure results in a zero return value.  In this case, a
> transient allocation failure error will likely be retried the next time the
> server responds with NFS4ERR_MOVED.
> 
> We can fixup the smatch warning with a small refactor: attempt all three
> allocations before testing and returning on a failure.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: c3ed222745d9 ("NFSv4: Fix free of uninitialized nfs4_label on referral lookup.")
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> Chuck, does this look sane?  I don't have a simple way to test this at the
> moment.  Also, I think the only result of returning -ENOMEM here instead
> would be that we skip continuing to try to migrate for other filesystems on
> this client, and we'd get a log message and trace output of the failure.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


>  fs/nfs/nfs4state.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 662e86ea3a2d..5b452411e8fd 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -2116,6 +2116,7 @@ static int nfs4_try_migration(struct nfs_server *server, const struct cred *cred
>  {
>  	struct nfs_client *clp = server->nfs_client;
>  	struct nfs4_fs_locations *locations = NULL;
> +	struct nfs_fattr *fattr;
>  	struct inode *inode;
>  	struct page *page;
>  	int status, result;
> @@ -2125,19 +2126,16 @@ static int nfs4_try_migration(struct nfs_server *server, const struct cred *cred
>  			(unsigned long long)server->fsid.minor,
>  			clp->cl_hostname);
>  
> -	result = 0;
>  	page = alloc_page(GFP_KERNEL);
>  	locations = kmalloc(sizeof(struct nfs4_fs_locations), GFP_KERNEL);
> -	if (page == NULL || locations == NULL) {
> -		dprintk("<-- %s: no memory\n", __func__);
> -		goto out;
> -	}
> -	locations->fattr = nfs_alloc_fattr();
> -	if (locations->fattr == NULL) {
> +	fattr = nfs_alloc_fattr();
> +	if (page == NULL || locations == NULL || fattr == NULL) {
>  		dprintk("<-- %s: no memory\n", __func__);
> +		result = 0;
>  		goto out;
>  	}
>  
> +	locations->fattr = fattr;
>  	inode = d_inode(server->super->s_root);
>  	result = nfs4_proc_get_locations(server, NFS_FH(inode), locations,
>  					 page, cred);
> -- 
> 2.44.0
> 

-- 
Chuck Lever