* [bug report] udf: merge bh free
@ 2025-03-11 12:35 Dan Carpenter
2025-03-12 15:15 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2025-03-11 12:35 UTC (permalink / raw)
To: Fabian Frederick; +Cc: Jan Kara, linux-fsdevel
Hello Fabian Frederick,
Commit 02d4ca49fa22 ("udf: merge bh free") from Jan 6, 2017
(linux-next), leads to the following Smatch static checker warning:
fs/udf/namei.c:442 udf_mkdir()
warn: passing positive error code '(-117),(-28),(-22),(-12),(-5),(-1),1' to 'ERR_PTR'
fs/udf/namei.c
422 static struct dentry *udf_mkdir(struct mnt_idmap *idmap, struct inode *dir,
423 struct dentry *dentry, umode_t mode)
424 {
425 struct inode *inode;
426 struct udf_fileident_iter iter;
427 int err;
428 struct udf_inode_info *dinfo = UDF_I(dir);
429 struct udf_inode_info *iinfo;
430
431 inode = udf_new_inode(dir, S_IFDIR | mode);
432 if (IS_ERR(inode))
433 return ERR_CAST(inode);
434
435 iinfo = UDF_I(inode);
436 inode->i_op = &udf_dir_inode_operations;
437 inode->i_fop = &udf_dir_operations;
438 err = udf_fiiter_add_entry(inode, NULL, &iter);
439 if (err) {
440 clear_nlink(inode);
441 discard_new_inode(inode);
--> 442 return ERR_PTR(err);
Returning ERR_PTR(1) will lead to an Oops in the caller.
443 }
The issue is this code from inode_getblk():
fs/udf/inode.c
787 /*
788 * Move prev_epos and cur_epos into indirect extent if we are at
789 * the pointer to it
790 */
791 ret = udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
792 if (ret < 0)
793 goto out_free;
794 ret = udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
^^^^^^^^^^^^^^^^^^^
ret is set here. It can be a negative error code, zero for EOF or one
on success.
795 if (ret < 0)
796 goto out_free;
797
798 /* if the extent is allocated and recorded, return the block
799 if the extent is not a multiple of the blocksize, round up */
800
801 if (!isBeyondEOF && etype == (EXT_RECORDED_ALLOCATED >> 30)) {
802 if (elen & (inode->i_sb->s_blocksize - 1)) {
803 elen = EXT_RECORDED_ALLOCATED |
804 ((elen + inode->i_sb->s_blocksize - 1) &
805 ~(inode->i_sb->s_blocksize - 1));
806 iinfo->i_lenExtents =
807 ALIGN(iinfo->i_lenExtents,
808 inode->i_sb->s_blocksize);
809 udf_write_aext(inode, &cur_epos, &eloc, elen, 1);
810 }
811 map->oflags = UDF_BLK_MAPPED;
812 map->pblk = udf_get_lb_pblock(inode->i_sb, &eloc, offset);
813 goto out_free;
Smatch is concerned that the ret = 1 from this goto out_free gets
propagated back to the caller.
814 }
815
This seems intentional. The caller has similar code earlier earlier but
it won't be triggered on this path because UDF_MAP_CREATE is set.
405 static int udf_map_block(struct inode *inode, struct udf_map_rq *map)
406 {
407 int ret;
408 struct udf_inode_info *iinfo = UDF_I(inode);
409
410 if (WARN_ON_ONCE(iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB))
411 return -EFSCORRUPTED;
412
413 map->oflags = 0;
414 if (!(map->iflags & UDF_MAP_CREATE)) {
415 struct kernel_lb_addr eloc;
416 uint32_t elen;
417 sector_t offset;
418 struct extent_position epos = {};
419 int8_t etype;
420
421 down_read(&iinfo->i_data_sem);
422 ret = inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset,
423 &etype);
424 if (ret < 0)
425 goto out_read;
426 if (ret > 0 && etype == (EXT_RECORDED_ALLOCATED >> 30)) {
427 map->pblk = udf_get_lb_pblock(inode->i_sb, &eloc,
428 offset);
429 map->oflags |= UDF_BLK_MAPPED;
430 ret = 0;
431 }
432 out_read:
433 up_read(&iinfo->i_data_sem);
434 brelse(epos.bh);
435
436 return ret;
ret could be either zero or one here.
437 }
It's unclear what to do...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] udf: merge bh free
2025-03-11 12:35 [bug report] udf: merge bh free Dan Carpenter
@ 2025-03-12 15:15 ` Jan Kara
2025-03-12 15:32 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2025-03-12 15:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Fabian Frederick, Jan Kara, linux-fsdevel
Hello Dan!
On Tue 11-03-25 15:35:20, Dan Carpenter wrote:
> Commit 02d4ca49fa22 ("udf: merge bh free") from Jan 6, 2017
> (linux-next), leads to the following Smatch static checker warning:
Thanks for the report! I think you've misidentified the commit introducing
the problem. The problem comes from a much more recent b405c1e58b73 ("udf:
refactor udf_next_aext() to handle error") which started to set 'ret' on
that path. But that's just a minor issue.
> fs/udf/namei.c:442 udf_mkdir()
> warn: passing positive error code '(-117),(-28),(-22),(-12),(-5),(-1),1' to 'ERR_PTR'
>
> fs/udf/namei.c
> 422 static struct dentry *udf_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> 423 struct dentry *dentry, umode_t mode)
> 424 {
> 425 struct inode *inode;
> 426 struct udf_fileident_iter iter;
> 427 int err;
> 428 struct udf_inode_info *dinfo = UDF_I(dir);
> 429 struct udf_inode_info *iinfo;
> 430
> 431 inode = udf_new_inode(dir, S_IFDIR | mode);
> 432 if (IS_ERR(inode))
> 433 return ERR_CAST(inode);
> 434
> 435 iinfo = UDF_I(inode);
> 436 inode->i_op = &udf_dir_inode_operations;
> 437 inode->i_fop = &udf_dir_operations;
> 438 err = udf_fiiter_add_entry(inode, NULL, &iter);
> 439 if (err) {
> 440 clear_nlink(inode);
> 441 discard_new_inode(inode);
> --> 442 return ERR_PTR(err);
>
> Returning ERR_PTR(1) will lead to an Oops in the caller.
Yeah, not good.
> The issue is this code from inode_getblk():
>
> fs/udf/inode.c
> 787 /*
> 788 * Move prev_epos and cur_epos into indirect extent if we are at
> 789 * the pointer to it
> 790 */
> 791 ret = udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
> 792 if (ret < 0)
> 793 goto out_free;
> 794 ret = udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
> ^^^^^^^^^^^^^^^^^^^
> ret is set here. It can be a negative error code, zero for EOF or one
> on success.
>
> 795 if (ret < 0)
> 796 goto out_free;
> 797
> 798 /* if the extent is allocated and recorded, return the block
> 799 if the extent is not a multiple of the blocksize, round up */
> 800
> 801 if (!isBeyondEOF && etype == (EXT_RECORDED_ALLOCATED >> 30)) {
> 802 if (elen & (inode->i_sb->s_blocksize - 1)) {
> 803 elen = EXT_RECORDED_ALLOCATED |
> 804 ((elen + inode->i_sb->s_blocksize - 1) &
> 805 ~(inode->i_sb->s_blocksize - 1));
> 806 iinfo->i_lenExtents =
> 807 ALIGN(iinfo->i_lenExtents,
> 808 inode->i_sb->s_blocksize);
> 809 udf_write_aext(inode, &cur_epos, &eloc, elen, 1);
> 810 }
> 811 map->oflags = UDF_BLK_MAPPED;
> 812 map->pblk = udf_get_lb_pblock(inode->i_sb, &eloc, offset);
> 813 goto out_free;
>
> Smatch is concerned that the ret = 1 from this goto out_free gets
> propagated back to the caller.
Indeed. We should have set ret = 0 here to comply with the calling
convention of inode_getblk() which is 0 on success < 0 on error. I'll send
a fix.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] udf: merge bh free
2025-03-12 15:15 ` Jan Kara
@ 2025-03-12 15:32 ` Jan Kara
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2025-03-12 15:32 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Fabian Frederick, Jan Kara, linux-fsdevel
On Wed 12-03-25 16:15:53, Jan Kara wrote:
> Hello Dan!
>
> On Tue 11-03-25 15:35:20, Dan Carpenter wrote:
> > Commit 02d4ca49fa22 ("udf: merge bh free") from Jan 6, 2017
> > (linux-next), leads to the following Smatch static checker warning:
>
> Thanks for the report! I think you've misidentified the commit introducing
> the problem. The problem comes from a much more recent b405c1e58b73 ("udf:
> refactor udf_next_aext() to handle error") which started to set 'ret' on
> that path. But that's just a minor issue.
>
> > fs/udf/namei.c:442 udf_mkdir()
> > warn: passing positive error code '(-117),(-28),(-22),(-12),(-5),(-1),1' to 'ERR_PTR'
> >
> > fs/udf/namei.c
> > 422 static struct dentry *udf_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > 423 struct dentry *dentry, umode_t mode)
> > 424 {
> > 425 struct inode *inode;
> > 426 struct udf_fileident_iter iter;
> > 427 int err;
> > 428 struct udf_inode_info *dinfo = UDF_I(dir);
> > 429 struct udf_inode_info *iinfo;
> > 430
> > 431 inode = udf_new_inode(dir, S_IFDIR | mode);
> > 432 if (IS_ERR(inode))
> > 433 return ERR_CAST(inode);
> > 434
> > 435 iinfo = UDF_I(inode);
> > 436 inode->i_op = &udf_dir_inode_operations;
> > 437 inode->i_fop = &udf_dir_operations;
> > 438 err = udf_fiiter_add_entry(inode, NULL, &iter);
> > 439 if (err) {
> > 440 clear_nlink(inode);
> > 441 discard_new_inode(inode);
> > --> 442 return ERR_PTR(err);
> >
> > Returning ERR_PTR(1) will lead to an Oops in the caller.
>
> Yeah, not good.
BTW, I've realized this is not really possible to hit in practice because
udf_fiiter_add_entry() calls udf_bread() (and thus inode_getblk()) for
known unallocated block and thus the path in inode_getblk() with the wrong
return value will not be executed in this case. Still this is rather
dangerous bug and better have it fixed.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-12 15:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-11 12:35 [bug report] udf: merge bh free Dan Carpenter
2025-03-12 15:15 ` Jan Kara
2025-03-12 15:32 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).