Hello Tigran Mkrtchyan,
Commit f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS
errors") from Jun 27, 2025 (linux-next), leads to the following
Smatch static checker warning:
fs/nfs/flexfilelayout/flexfilelayout.c:880 ff_layout_pg_init_read()
error: uninitialized symbol 'ds_idx'.
We recently changed ff_layout_choose_ds_for_read() from returning NULL to
returning error pointers. There are two bugs. One error path in
ff_layout_choose_ds_for_read() accidentally returns success. And some
of the callers are still checking for NULL instead of error pointers.
Btw, I'm always promoting my blog about error pointers and NULL:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
It's not really related here, since we should not mix error pointers
and NULL but I still link to it because that's what they taught me in
my Trump Wealth Institute course on Search Engine Optimization.
Here is what ff_layout_choose_ds_for_read() looks like:
fs/nfs/flexfilelayout/flexfilelayout.c
758 static struct nfs4_pnfs_ds *
759 ff_layout_choose_ds_for_read(struct pnfs_layout_segment *lseg,
760 u32 start_idx, u32 *best_idx,
761 bool check_device)
762 {
763 struct nfs4_ff_layout_segment *fls = FF_LAYOUT_LSEG(lseg);
764 struct nfs4_ff_layout_mirror *mirror;
765 struct nfs4_pnfs_ds *ds = ERR_PTR(-EAGAIN);
766 u32 idx;
767
768 /* mirrors are initially sorted by efficiency */
769 for (idx = start_idx; idx < fls->mirror_array_cnt; idx++) {
770 mirror = FF_LAYOUT_COMP(lseg, idx);
771 ds = nfs4_ff_layout_prepare_ds(lseg, mirror, false);
772 if (IS_ERR(ds))
773 continue;
774
775 if (check_device &&
776 nfs4_test_deviceid_unavailable(&mirror->mirror_ds->id_node))
777 continue;
Bug #1: If we hit this continue on the last iteration through the loop
then we return success and *best_idx is not initialized. It should be:
if (check_device &&
nfs4_test_deviceid_unavailable(&mirror->mirror_ds->id_node)) {
ds = ERR_PTR(-EINVAL);
continue;
}
778
779 *best_idx = idx;
780 break;
781 }
782
783 return ds;
784 }
Bug #2: The whole rest of the call tree expects NULL and not error pointers.
For example, ff_layout_get_ds_for_read():
fs/nfs/flexfilelayout/flexfilelayout.c
812 static struct nfs4_pnfs_ds *
813 ff_layout_get_ds_for_read(struct nfs_pageio_descriptor *pgio,
814 u32 *best_idx)
815 {
816 struct pnfs_layout_segment *lseg = pgio->pg_lseg;
817 struct nfs4_pnfs_ds *ds;
818
819 ds = ff_layout_choose_best_ds_for_read(lseg, pgio->pg_mirror_idx,
820 best_idx);
821 if (ds || !pgio->pg_mirror_idx)
^^
822 return ds;
This needs to check for error pointers. But also if pgio->pg_mirror_idx
is zero, is that a success return? I don't know...
823 return ff_layout_choose_best_ds_for_read(lseg, 0, best_idx);
824 }
fs/nfs/flexfilelayout/flexfilelayout.c
842 static void
843 ff_layout_pg_init_read(struct nfs_pageio_descriptor *pgio,
844 struct nfs_page *req)
845 {
846 struct nfs_pgio_mirror *pgm;
847 struct nfs4_ff_layout_mirror *mirror;
848 struct nfs4_pnfs_ds *ds;
849 u32 ds_idx;
850
851 if (NFS_SERVER(pgio->pg_inode)->flags &
852 (NFS_MOUNT_SOFT|NFS_MOUNT_SOFTERR))
853 pgio->pg_maxretrans = io_maxretrans;
854 retry:
855 pnfs_generic_pg_check_layout(pgio, req);
856 /* Use full layout for now */
857 if (!pgio->pg_lseg) {
858 ff_layout_pg_get_read(pgio, req, false);
859 if (!pgio->pg_lseg)
860 goto out_nolseg;
861 }
862 if (ff_layout_avoid_read_on_rw(pgio->pg_lseg)) {
863 ff_layout_pg_get_read(pgio, req, true);
864 if (!pgio->pg_lseg)
865 goto out_nolseg;
866 }
867 /* Reset wb_nio, since getting layout segment was successful */
868 req->wb_nio = 0;
869
870 ds = ff_layout_get_ds_for_read(pgio, &ds_idx);
871 if (!ds) {
^^^^^^^^^^
This doesn't check for error pointers either.
872 if (!ff_layout_no_fallback_to_mds(pgio->pg_lseg))
873 goto out_mds;
874 pnfs_generic_pg_cleanup(pgio);
875 /* Sleep for 1 second before retrying */
876 ssleep(1);
877 goto retry;
878 }
879
--> 880 mirror = FF_LAYOUT_COMP(pgio->pg_lseg, ds_idx);
^^^^^^
Uninitialized.
regards,
dan carpenter
