[PATCH] flexfiles/pNFS: fix NULL checks on result of ff_layout_choose_ds_for_read

[PATCH] flexfiles/pNFS: fix NULL checks on result of ff_layout_choose_ds_for_read

[Tigran Mkrtchyan]

Recent commit f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS
errors") has changed the error return type of ff_layout_choose_ds_for_read() from
NULL to an error pointer. However, not all code paths have been updated
to match the change. Thus, some non-NULL checks will accept error pointers
as a valid return value.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS errors")
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 8dc921d83538..258158dfd557 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -804,7 +804,7 @@ ff_layout_choose_best_ds_for_read(struct pnfs_layout_segment *lseg,
 	struct nfs4_pnfs_ds *ds;
 
 	ds = ff_layout_choose_valid_ds_for_read(lseg, start_idx, best_idx);
-	if (ds)
+	if (!IS_ERR(ds))
 		return ds;
 	return ff_layout_choose_any_ds_for_read(lseg, start_idx, best_idx);
 }
@@ -818,7 +818,7 @@ ff_layout_get_ds_for_read(struct nfs_pageio_descriptor *pgio,
 
 	ds = ff_layout_choose_best_ds_for_read(lseg, pgio->pg_mirror_idx,
 					       best_idx);
-	if (ds || !pgio->pg_mirror_idx)
+	if (!IS_ERR(ds) || !pgio->pg_mirror_idx)
 		return ds;
 	return ff_layout_choose_best_ds_for_read(lseg, 0, best_idx);
 }
@@ -868,7 +868,7 @@ ff_layout_pg_init_read(struct nfs_pageio_descriptor *pgio,
 	req->wb_nio = 0;
 
 	ds = ff_layout_get_ds_for_read(pgio, &ds_idx);
-	if (!ds) {
+	if (IS_ERR(ds)) {
 		if (!ff_layout_no_fallback_to_mds(pgio->pg_lseg))
 			goto out_mds;
 		pnfs_generic_pg_cleanup(pgio);
-- 
2.51.0

Re: [PATCH] flexfiles/pNFS: fix NULL checks on result of ff_layout_choose_ds_for_read

[Dan Carpenter]

On Thu, Aug 28, 2025 at 03:38:43PM +0200, Tigran Mkrtchyan wrote:
> Recent commit f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS
> errors") has changed the error return type of ff_layout_choose_ds_for_read() from
> NULL to an error pointer. However, not all code paths have been updated
> to match the change. Thus, some non-NULL checks will accept error pointers
> as a valid return value.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS errors")
> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
> ---

This is still not complete.

  1073  static void ff_layout_resend_pnfs_read(struct nfs_pgio_header *hdr)
  1074  {
  1075          u32 idx = hdr->pgio_mirror_idx + 1;
  1076          u32 new_idx = 0;
  1077  
  1078          if (ff_layout_choose_any_ds_for_read(hdr->lseg, idx, &new_idx))
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be:

	struct nfs4_pnfs_ds *ds;

	ds = ff_layout_choose_any_ds_for_read(hdr->lseg, idx, &new_idx);
	if (IS_ERR(ds))
		pnfs_error_mark_layout_for_return(hdr->inode, hdr->lseg);
	else
		ff_layout_send_layouterror(hdr->lseg);

  1079                  ff_layout_send_layouterror(hdr->lseg);
  1080          else
  1081                  pnfs_error_mark_layout_for_return(hdr->inode, hdr->lseg);
  1082          pnfs_read_resend_pnfs(hdr, new_idx);
  1083  }

Also the continue in ff_layout_choose_ds_for_read() still needs to be
fixed as I mentioned earlier.

regards,
dan carpenter

Re: [PATCH] flexfiles/pNFS: fix NULL checks on result of ff_layout_choose_ds_for_read

[Mkrtchyan, Tigran]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]



----- Original Message -----
> From: "Dan Carpenter" <dan.carpenter@linaro.org>
> To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> Cc: "trondmy" <trondmy@kernel.org>, "anna schumaker" <anna.schumaker@oracle.com>, "linux-nfs"
> <linux-nfs@vger.kernel.org>
> Sent: Thursday, 28 August, 2025 16:38:29
> Subject: Re: [PATCH] flexfiles/pNFS: fix NULL checks on result of ff_layout_choose_ds_for_read

> On Thu, Aug 28, 2025 at 03:38:43PM +0200, Tigran Mkrtchyan wrote:
>> Recent commit f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS
>> errors") has changed the error return type of ff_layout_choose_ds_for_read()
>> from
>> NULL to an error pointer. However, not all code paths have been updated
>> to match the change. Thus, some non-NULL checks will accept error pointers
>> as a valid return value.
>> 
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Fixes: f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS errors")
>> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
>> ---
> 
> This is still not complete.
> 
>  1073  static void ff_layout_resend_pnfs_read(struct nfs_pgio_header *hdr)
>  1074  {
>  1075          u32 idx = hdr->pgio_mirror_idx + 1;
>  1076          u32 new_idx = 0;
>  1077
>  1078          if (ff_layout_choose_any_ds_for_read(hdr->lseg, idx, &new_idx))
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This should be:
> 
>	struct nfs4_pnfs_ds *ds;
> 
>	ds = ff_layout_choose_any_ds_for_read(hdr->lseg, idx, &new_idx);
>	if (IS_ERR(ds))
>		pnfs_error_mark_layout_for_return(hdr->inode, hdr->lseg);
>	else
>		ff_layout_send_layouterror(hdr->lseg);


Yeah, makes sense. Thanks!

Tigran.
> 
>  1079                  ff_layout_send_layouterror(hdr->lseg);
>  1080          else
>  1081                  pnfs_error_mark_layout_for_return(hdr->inode, hdr->lseg);
>  1082          pnfs_read_resend_pnfs(hdr, new_idx);
>  1083  }
> 
> Also the continue in ff_layout_choose_ds_for_read() still needs to be
> fixed as I mentioned earlier.
> 
> regards,
> dan carpenter

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 2826 bytes --]