* [bug report] fs/ntfs3: Add initialization of super block
@ 2021-08-24 7:58 Dan Carpenter
2021-08-24 10:33 ` Kari Argillander
2026-02-09 15:24 ` [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() Konstantin Komarov
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-08-24 7:58 UTC (permalink / raw)
To: almaz.alexandrovich; +Cc: ntfs3
Hello Konstantin Komarov,
The patch 82cae269cfa9: "fs/ntfs3: Add initialization of super block"
from Aug 13, 2021, leads to the following
Smatch static checker warning:
fs/ntfs3/index.c:238 bmp_buf_get()
warn: 'bh' could be an error pointer
fs/ntfs3/index.c
229 data_size = le64_to_cpu(b->nres.data_size);
230 if (WARN_ON(off >= data_size)) {
231 /* looks like filesystem error */
232 return -EINVAL;
233 }
234
235 valid_size = le64_to_cpu(b->nres.valid_size);
236
237 bh = ntfs_bread_run(sbi, &indx->bitmap_run, off);
--> 238 if (!bh)
239 return -EIO;
240
241 if (IS_ERR(bh))
This is not a bug, but it is wrong style. When a function returns both
error pointers and NULL then the NULL return is means the feature is
disabled. It's not an error. Just that the feature is turned off
deliberately in the Kconfig or whatever. Don't print an error message,
just continue with the feature disabled as the admin has requested.
But here NULL is just an error. The ntfs_bread_run() should do:
bh = ntfs_bread();
if (!bh)
return ERR_PTR(-EIO);
return bh;
242 return PTR_ERR(bh);
243
244 bbuf->bh = bh;
245
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] fs/ntfs3: Add initialization of super block 2021-08-24 7:58 [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter @ 2021-08-24 10:33 ` Kari Argillander 2026-02-09 15:24 ` [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() Konstantin Komarov 1 sibling, 0 replies; 5+ messages in thread From: Kari Argillander @ 2021-08-24 10:33 UTC (permalink / raw) To: Dan Carpenter; +Cc: almaz.alexandrovich, ntfs3 On Tue, Aug 24, 2021 at 10:58:19AM +0300, Dan Carpenter wrote: > Hello Konstantin Komarov, > > The patch 82cae269cfa9: "fs/ntfs3: Add initialization of super block" > from Aug 13, 2021, leads to the following > Smatch static checker warning: > > fs/ntfs3/index.c:238 bmp_buf_get() > warn: 'bh' could be an error pointer > > fs/ntfs3/index.c > 229 data_size = le64_to_cpu(b->nres.data_size); > 230 if (WARN_ON(off >= data_size)) { > 231 /* looks like filesystem error */ > 232 return -EINVAL; > 233 } > 234 > 235 valid_size = le64_to_cpu(b->nres.valid_size); > 236 > 237 bh = ntfs_bread_run(sbi, &indx->bitmap_run, off); > --> 238 if (!bh) > 239 return -EIO; > 240 > 241 if (IS_ERR(bh)) > > This is not a bug, but it is wrong style. When a function returns both > error pointers and NULL then the NULL return is means the feature is > disabled. It's not an error. Just that the feature is turned off > deliberately in the Kconfig or whatever. Don't print an error message, > just continue with the feature disabled as the admin has requested. > > But here NULL is just an error. The ntfs_bread_run() should do: Agreed. > > bh = ntfs_bread(); > if (!bh) > return ERR_PTR(-EIO); > return bh; Can also be like below but probably your version is more readable. return ntfs_bread(sb, lbo >> sb->s_blocksize_bits) ? : ERR_PTR(-EIO); > > 242 return PTR_ERR(bh); > 243 > 244 bbuf->bh = bh; > 245 > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() 2021-08-24 7:58 [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter 2021-08-24 10:33 ` Kari Argillander @ 2026-02-09 15:24 ` Konstantin Komarov 1 sibling, 0 replies; 5+ messages in thread From: Konstantin Komarov @ 2026-02-09 15:24 UTC (permalink / raw) To: ntfs3 Cc: linux-kernel, linux-fsdevel, Konstantin Komarov, kernel test robot, Dan Carpenter When ntfs_read_run_nb_ra() is invoked with run == NULL the code later assumes run is valid and may call run_get_entry(NULL, ...), and also uses clen/idx without initializing them. Smatch reported uninitialized variable warnings and this can lead to undefined behaviour. This patch fixes it. Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/r/202512230646.v5hrYXL0-lkp@intel.com/ Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> --- fs/ntfs3/fsntfs.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c index e9c39c62aea4..2ef500f1a9fa 100644 --- a/fs/ntfs3/fsntfs.c +++ b/fs/ntfs3/fsntfs.c @@ -1256,6 +1256,12 @@ int ntfs_read_run_nb_ra(struct ntfs_sb_info *sbi, const struct runs_tree *run, } while (len32); + if (!run) { + err = -EINVAL; + goto out; + } + + /* Get next fragment to read. */ vcn_next = vcn + clen; if (!run_get_entry(run, ++idx, &vcn, &lcn, &clen) || vcn != vcn_next) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <caa37f28-a2e8-4e0a-a9ce-a365ce805e4b@stanley.mountain>]
* [bug report] fs/ntfs3: Add initialization of super block [not found] <caa37f28-a2e8-4e0a-a9ce-a365ce805e4b@stanley.mountain> @ 2026-02-06 13:41 ` Dan Carpenter 2026-02-09 10:20 ` Konstantin Komarov 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2026-02-06 13:41 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel [ Smatch checking is paused while we raise funding. #SadFace https://lore.kernel.org/all/aTaiGSbWZ9DJaGo7@stanley.mountain/ -dan ] Hello Konstantin Komarov, Commit 82cae269cfa9 ("fs/ntfs3: Add initialization of super block") from Aug 13, 2021 (linux-next), leads to the following Smatch static checker warning: fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: we previously assumed 'run' could be null (see line 1178) fs/ntfs3/fsntfs.c:1259 ntfs_read_run_nb_ra() error: uninitialized symbol 'clen'. fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: uninitialized symbol 'idx'. fs/ntfs3/fsntfs.c 1161 int ntfs_read_run_nb_ra(struct ntfs_sb_info *sbi, const struct runs_tree *run, 1162 u64 vbo, void *buf, u32 bytes, struct ntfs_buffers *nb, 1163 struct file_ra_state *ra) 1164 { 1165 int err; 1166 struct super_block *sb = sbi->sb; 1167 struct address_space *mapping = sb->s_bdev->bd_mapping; 1168 u32 blocksize = sb->s_blocksize; 1169 u8 cluster_bits = sbi->cluster_bits; 1170 u32 off = vbo & sbi->cluster_mask; 1171 u32 nbh = 0; 1172 CLST vcn_next, vcn = vbo >> cluster_bits; 1173 CLST lcn, clen; 1174 u64 lbo, len; 1175 size_t idx; 1176 struct buffer_head *bh; 1177 1178 if (!run) { 1179 /* First reading of $Volume + $MFTMirr + $LogFile goes here. */ 1180 if (vbo > MFT_REC_VOL * sbi->record_size) { 1181 err = -ENOENT; 1182 goto out; 1183 } 1184 1185 /* Use absolute boot's 'MFTCluster' to read record. */ 1186 lbo = vbo + sbi->mft.lbo; 1187 len = sbi->record_size; If run is NULL then "clen" is uninitialized. 1188 } else if (!run_lookup_entry(run, vcn, &lcn, &clen, &idx)) { 1189 err = -ENOENT; 1190 goto out; 1191 } else { 1192 if (lcn == SPARSE_LCN) { 1193 err = -EINVAL; 1194 goto out; 1195 } 1196 1197 lbo = ((u64)lcn << cluster_bits) + off; 1198 len = ((u64)clen << cluster_bits) - off; 1199 } 1200 1201 off = lbo & (blocksize - 1); 1202 if (nb) { 1203 nb->off = off; 1204 nb->bytes = bytes; 1205 } 1206 1207 if (ra && !ra->ra_pages) 1208 file_ra_state_init(ra, mapping); 1209 1210 for (;;) { 1211 u32 len32 = len >= bytes ? bytes : len; 1212 sector_t block = lbo >> sb->s_blocksize_bits; 1213 1214 if (ra) { 1215 pgoff_t index = lbo >> PAGE_SHIFT; 1216 if (!ra_has_index(ra, index)) { 1217 page_cache_sync_readahead(mapping, ra, NULL, 1218 index, 1); 1219 ra->prev_pos = (loff_t)index << PAGE_SHIFT; 1220 } 1221 } 1222 1223 do { 1224 u32 op = blocksize - off; 1225 1226 if (op > len32) 1227 op = len32; 1228 1229 bh = ntfs_bread(sb, block); 1230 if (!bh) { 1231 err = -EIO; 1232 goto out; 1233 } 1234 1235 if (buf) { 1236 memcpy(buf, bh->b_data + off, op); 1237 buf = Add2Ptr(buf, op); 1238 } 1239 1240 if (!nb) { 1241 put_bh(bh); 1242 } else if (nbh >= ARRAY_SIZE(nb->bh)) { 1243 err = -EINVAL; 1244 goto out; 1245 } else { 1246 nb->bh[nbh++] = bh; 1247 nb->nbufs = nbh; 1248 } 1249 1250 bytes -= op; 1251 if (!bytes) 1252 return 0; 1253 len32 -= op; 1254 block += 1; 1255 off = 0; 1256 1257 } while (len32); 1258 --> 1259 vcn_next = vcn + clen; ^^^^ Used uninitalized here. 1260 if (!run_get_entry(run, ++idx, &vcn, &lcn, &clen) || But also if we pass a NULL run to run_get_entry() it will crash. I'm a bit confused by this code. 1261 vcn != vcn_next) { 1262 err = -ENOENT; 1263 goto out; 1264 } 1265 1266 if (lcn == SPARSE_LCN) { 1267 err = -EINVAL; 1268 goto out; 1269 } 1270 1271 lbo = ((u64)lcn << cluster_bits); 1272 len = ((u64)clen << cluster_bits); 1273 } 1274 1275 out: 1276 if (!nbh) 1277 return err; 1278 1279 while (nbh) { 1280 put_bh(nb->bh[--nbh]); 1281 nb->bh[nbh] = NULL; 1282 } 1283 1284 nb->nbufs = 0; 1285 return err; 1286 } regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] fs/ntfs3: Add initialization of super block 2026-02-06 13:41 ` [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter @ 2026-02-09 10:20 ` Konstantin Komarov 0 siblings, 0 replies; 5+ messages in thread From: Konstantin Komarov @ 2026-02-09 10:20 UTC (permalink / raw) To: Dan Carpenter; +Cc: ntfs3, linux-kernel On 2/6/26 14:41, Dan Carpenter wrote: > [ Smatch checking is paused while we raise funding. #SadFace > https://lore.kernel.org/all/aTaiGSbWZ9DJaGo7@stanley.mountain/ -dan ] > > Hello Konstantin Komarov, > > Commit 82cae269cfa9 ("fs/ntfs3: Add initialization of super block") > from Aug 13, 2021 (linux-next), leads to the following Smatch static > checker warning: > > fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: we previously assumed 'run' could be null (see line 1178) > fs/ntfs3/fsntfs.c:1259 ntfs_read_run_nb_ra() error: uninitialized symbol 'clen'. > fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: uninitialized symbol 'idx'. > > fs/ntfs3/fsntfs.c > 1161 int ntfs_read_run_nb_ra(struct ntfs_sb_info *sbi, const struct runs_tree *run, > 1162 u64 vbo, void *buf, u32 bytes, struct ntfs_buffers *nb, > 1163 struct file_ra_state *ra) > 1164 { > 1165 int err; > 1166 struct super_block *sb = sbi->sb; > 1167 struct address_space *mapping = sb->s_bdev->bd_mapping; > 1168 u32 blocksize = sb->s_blocksize; > 1169 u8 cluster_bits = sbi->cluster_bits; > 1170 u32 off = vbo & sbi->cluster_mask; > 1171 u32 nbh = 0; > 1172 CLST vcn_next, vcn = vbo >> cluster_bits; > 1173 CLST lcn, clen; > 1174 u64 lbo, len; > 1175 size_t idx; > 1176 struct buffer_head *bh; > 1177 > 1178 if (!run) { > 1179 /* First reading of $Volume + $MFTMirr + $LogFile goes here. */ > 1180 if (vbo > MFT_REC_VOL * sbi->record_size) { > 1181 err = -ENOENT; > 1182 goto out; > 1183 } > 1184 > 1185 /* Use absolute boot's 'MFTCluster' to read record. */ > 1186 lbo = vbo + sbi->mft.lbo; > 1187 len = sbi->record_size; > > If run is NULL then "clen" is uninitialized. > > 1188 } else if (!run_lookup_entry(run, vcn, &lcn, &clen, &idx)) { > 1189 err = -ENOENT; > 1190 goto out; > 1191 } else { > 1192 if (lcn == SPARSE_LCN) { > 1193 err = -EINVAL; > 1194 goto out; > 1195 } > 1196 > 1197 lbo = ((u64)lcn << cluster_bits) + off; > 1198 len = ((u64)clen << cluster_bits) - off; > 1199 } > 1200 > 1201 off = lbo & (blocksize - 1); > 1202 if (nb) { > 1203 nb->off = off; > 1204 nb->bytes = bytes; > 1205 } > 1206 > 1207 if (ra && !ra->ra_pages) > 1208 file_ra_state_init(ra, mapping); > 1209 > 1210 for (;;) { > 1211 u32 len32 = len >= bytes ? bytes : len; > 1212 sector_t block = lbo >> sb->s_blocksize_bits; > 1213 > 1214 if (ra) { > 1215 pgoff_t index = lbo >> PAGE_SHIFT; > 1216 if (!ra_has_index(ra, index)) { > 1217 page_cache_sync_readahead(mapping, ra, NULL, > 1218 index, 1); > 1219 ra->prev_pos = (loff_t)index << PAGE_SHIFT; > 1220 } > 1221 } > 1222 > 1223 do { > 1224 u32 op = blocksize - off; > 1225 > 1226 if (op > len32) > 1227 op = len32; > 1228 > 1229 bh = ntfs_bread(sb, block); > 1230 if (!bh) { > 1231 err = -EIO; > 1232 goto out; > 1233 } > 1234 > 1235 if (buf) { > 1236 memcpy(buf, bh->b_data + off, op); > 1237 buf = Add2Ptr(buf, op); > 1238 } > 1239 > 1240 if (!nb) { > 1241 put_bh(bh); > 1242 } else if (nbh >= ARRAY_SIZE(nb->bh)) { > 1243 err = -EINVAL; > 1244 goto out; > 1245 } else { > 1246 nb->bh[nbh++] = bh; > 1247 nb->nbufs = nbh; > 1248 } > 1249 > 1250 bytes -= op; > 1251 if (!bytes) > 1252 return 0; > 1253 len32 -= op; > 1254 block += 1; > 1255 off = 0; > 1256 > 1257 } while (len32); > 1258 > --> 1259 vcn_next = vcn + clen; > ^^^^ > Used uninitalized here. > > 1260 if (!run_get_entry(run, ++idx, &vcn, &lcn, &clen) || > > But also if we pass a NULL run to run_get_entry() it will crash. I'm > a bit confused by this code. > > 1261 vcn != vcn_next) { > 1262 err = -ENOENT; > 1263 goto out; > 1264 } > 1265 > 1266 if (lcn == SPARSE_LCN) { > 1267 err = -EINVAL; > 1268 goto out; > 1269 } > 1270 > 1271 lbo = ((u64)lcn << cluster_bits); > 1272 len = ((u64)clen << cluster_bits); > 1273 } > 1274 > 1275 out: > 1276 if (!nbh) > 1277 return err; > 1278 > 1279 while (nbh) { > 1280 put_bh(nb->bh[--nbh]); > 1281 nb->bh[nbh] = NULL; > 1282 } > 1283 > 1284 nb->nbufs = 0; > 1285 return err; > 1286 } > > regards, > dan carpenter Hello, Thanks for the Smatch report. I’ll examine the warnings, prepare a fix, and post a patch. Regards, Konstantin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-09 15:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-24 7:58 [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
2021-08-24 10:33 ` Kari Argillander
2026-02-09 15:24 ` [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() Konstantin Komarov
[not found] <caa37f28-a2e8-4e0a-a9ce-a365ce805e4b@stanley.mountain>
2026-02-06 13:41 ` [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
2026-02-09 10:20 ` Konstantin Komarov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox