Re: [PATCH 3/7] udf: Handle error when adding extent to a file

[Jan Kara <jack@suse.cz>]

On Sun 25-12-22 21:28:43, Nathan Chancellor wrote:
> On Thu, Dec 22, 2022 at 11:16:00AM +0100, Jan Kara wrote:
> > When adding extent to a file fails, so far we've silently squelshed the
> > error. Make sure to propagate it up properly.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/udf/inode.c | 41 +++++++++++++++++++++++++++--------------
> >  1 file changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> > index 09417342d8b6..15b3e529854b 100644
> > --- a/fs/udf/inode.c
> > +++ b/fs/udf/inode.c
> > @@ -57,15 +57,15 @@ static int udf_update_inode(struct inode *, int);
> >  static int udf_sync_inode(struct inode *inode);
> >  static int udf_alloc_i_data(struct inode *inode, size_t size);
> >  static sector_t inode_getblk(struct inode *, sector_t, int *, int *);
> > -static int8_t udf_insert_aext(struct inode *, struct extent_position,
> > -			      struct kernel_lb_addr, uint32_t);
> > +static int udf_insert_aext(struct inode *, struct extent_position,
> > +			   struct kernel_lb_addr, uint32_t);
> >  static void udf_split_extents(struct inode *, int *, int, udf_pblk_t,
> >  			      struct kernel_long_ad *, int *);
> >  static void udf_prealloc_extents(struct inode *, int, int,
> >  				 struct kernel_long_ad *, int *);
> >  static void udf_merge_extents(struct inode *, struct kernel_long_ad *, int *);
> > -static void udf_update_extents(struct inode *, struct kernel_long_ad *, int,
> > -			       int, struct extent_position *);
> > +static int udf_update_extents(struct inode *, struct kernel_long_ad *, int,
> > +			      int, struct extent_position *);
> >  static int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
> >  
> >  static void __udf_clear_extent_cache(struct inode *inode)
> > @@ -795,7 +795,9 @@ static sector_t inode_getblk(struct inode *inode, sector_t block,
> >  	/* write back the new extents, inserting new extents if the new number
> >  	 * of extents is greater than the old number, and deleting extents if
> >  	 * the new number of extents is less than the old number */
> > -	udf_update_extents(inode, laarr, startnum, endnum, &prev_epos);
> > +	*err = udf_update_extents(inode, laarr, startnum, endnum, &prev_epos);
> > +	if (*err < 0)
> > +		goto out_free;
> 
> This patch in next-20221226 as commit d8b39db5fab8 ("udf: Handle error when
> adding extent to a file") causes the following clang warning:
> 
>   fs/udf/inode.c:805:6: error: variable 'newblock' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>           if (*err < 0)
>               ^~~~~~~~
>   fs/udf/inode.c:827:9: note: uninitialized use occurs here
>           return newblock;
>                  ^~~~~~~~
>   fs/udf/inode.c:805:2: note: remove the 'if' if its condition is always false
>           if (*err < 0)
>           ^~~~~~~~~~~~~
>   fs/udf/inode.c:607:34: note: initialize the variable 'newblock' to silence this warning
>           udf_pblk_t newblocknum, newblock;
>                                           ^
>                                            = 0
>   1 error generated.

Thanks for the report. It should be fixed now.

								Honza


> 
> >  	newblock = udf_get_pblock(inode->i_sb, newblocknum,
> >  				iinfo->i_location.partitionReferenceNum, 0);
> > @@ -1063,21 +1065,30 @@ static void udf_merge_extents(struct inode *inode, struct kernel_long_ad *laarr,
> >  	}
> >  }
> >  
> > -static void udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
> > -			       int startnum, int endnum,
> > -			       struct extent_position *epos)
> > +static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
> > +			      int startnum, int endnum,
> > +			      struct extent_position *epos)
> >  {
> >  	int start = 0, i;
> >  	struct kernel_lb_addr tmploc;
> >  	uint32_t tmplen;
> > +	int err;
> >  
> >  	if (startnum > endnum) {
> >  		for (i = 0; i < (startnum - endnum); i++)
> >  			udf_delete_aext(inode, *epos);
> >  	} else if (startnum < endnum) {
> >  		for (i = 0; i < (endnum - startnum); i++) {
> > -			udf_insert_aext(inode, *epos, laarr[i].extLocation,
> > -					laarr[i].extLength);
> > +			err = udf_insert_aext(inode, *epos,
> > +					      laarr[i].extLocation,
> > +					      laarr[i].extLength);
> > +			/*
> > +			 * If we fail here, we are likely corrupting the extent
> > + 			 * list and leaking blocks. At least stop early to
> > +			 * limit the damage.
> > +			 */
> > +			if (err < 0)
> > +				return err;
> >  			udf_next_aext(inode, epos, &laarr[i].extLocation,
> >  				      &laarr[i].extLength, 1);
> >  			start++;
> > @@ -1089,6 +1100,7 @@ static void udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr
> >  		udf_write_aext(inode, epos, &laarr[i].extLocation,
> >  			       laarr[i].extLength, 1);
> >  	}
> > +	return 0;
> >  }
> >  
> >  struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
> > @@ -2107,12 +2119,13 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
> >  	return etype;
> >  }
> >  
> > -static int8_t udf_insert_aext(struct inode *inode, struct extent_position epos,
> > -			      struct kernel_lb_addr neloc, uint32_t nelen)
> > +static int udf_insert_aext(struct inode *inode, struct extent_position epos,
> > +			   struct kernel_lb_addr neloc, uint32_t nelen)
> >  {
> >  	struct kernel_lb_addr oeloc;
> >  	uint32_t oelen;
> >  	int8_t etype;
> > +	int err;
> >  
> >  	if (epos.bh)
> >  		get_bh(epos.bh);
> > @@ -2122,10 +2135,10 @@ static int8_t udf_insert_aext(struct inode *inode, struct extent_position epos,
> >  		neloc = oeloc;
> >  		nelen = (etype << 30) | oelen;
> >  	}
> > -	udf_add_aext(inode, &epos, &neloc, nelen, 1);
> > +	err = udf_add_aext(inode, &epos, &neloc, nelen, 1);
> >  	brelse(epos.bh);
> >  
> > -	return (nelen >> 30);
> > +	return err;
> >  }
> >  
> >  int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
> > -- 
> > 2.35.3
> > 
> > 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR