[U-Boot] ext4 delete file fails when ext4 extents enabled in filesystem

[Brüns, Stefan]

On Freitag, 2. September 2016 14:51:59 CEST Thomas Schaefer wrote:
> > -----Ursprüngliche Nachricht-----
> > Von: Brüns, Stefan [mailto:Stefan.Bruens at rwth-aachen.de]
> > Gesendet: Freitag, 2. September 2016 13:43
> > An: Thomas Schaefer
> > Cc: u-boot at lists.denx.de; Michael Walle
> > Betreff: Re: ext4 delete file fails when ext4 extents enabled in
> > filesystem
> > 
> > On Donnerstag, 1. September 2016 19:25:30 CEST you wrote:
> > > On Donnerstag, 1. September 2016 16:08:51 CEST you wrote:
> > > > Hi Stefan,
> > > > 
> > > > applying patch [U-Boot,v4,06/13]ext4 and Michael Walles patch
> > > > [U-Boot,v4,3/4]ext4, I'm now able to write into directories on ext4
> > > > fs from u-boot. However, when deleting a given file (i.e. when
> > > > writing to an existing filename), u-boot crashes when ext4 extents
> > > > are enabled.
> > > > 
> > > > Some debugging showd that blknr from 'read_allocated_block' function
> > > > returns negative value. I can only guess, maybe its due to 64 bit
> > > > values calculated from ee_start_hi and ee_start_lo entries in the
> > > > ext4_extent structure.
> > > > 
> > > > When disabling extents in the ext4 fs, deleting a given file is
> > > > working.
[...]
> > Hi Thomas,
> > 
> > short followup:
> > 
> > read_allocated_blocks returns either 0 or -1 in case of an error.
> > Unfortunately, the return value is only checked for 0 equality in
> > most/all?
> > cases, and seemingly my patch series introduced some more occasions.
> > 
> > 
> > Now, what *should* read_allocated_blocks return in case of an error?
> > Either:
> > 
> > - 0: a file block can never be allocated as block 0, as that is always in
> > use by the superblock and/or the bootsector block.
> > 
> > - <0: Extents allow 48 bit block numbers. "Limiting" the return value to
> > the positive half of int64_t for valid block numbers and and reserving
> > negative values for error codes is fine.
> > 
> > I would go for negative error codes, as these are more expressive.
> > Comments/ opinions welcome!

Following up on this, the correct behaviour is <0 on "real" errors, like -
ENOMEM, and 0 on blocks not backed on device (e.g. sparse files).

Followup patch in progress.


[...]
> Hi Stefan,
> 
> the attachment contains an image file that causes u-boot to crash when
> trying to overwrite existing files in ext4 fs.

Could reproduce this. The problem seems to be an out-of-bound access of an in-
memory struct ext2_inode, and mixing up its size with fs->inodesz. I have 
found at least one place, will investigate further.

Kind regards,

Stefan