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@xxxxxxxx>
SUSE Labs, CR
