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

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

[Thomas Schaefer]

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.


Best regards,
Thomas Sch?fer

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

[Brüns, Stefan]

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,
> 
> U-boots ext4 implementation currently does not support 64bit or even 48bit
> block numbers, so this may be the cause.
> 
> Can you provide some information about your test setup?
> 
> You can use the debugsfs ext tool to gather some information about the
> problematic file. Just access the filesystem with:
> 
> /sbin/debugfs /dev/sda1  ;  (or whatever your partion name is)
> 
>   or
> 
> /sbin/debugfs /path/to/imagefile
> 
> debugfs supports commands like cd, stat, ls. stat gives you the block number
> list.
> 
> Kind regards,
> 
> Stefan

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!


I will update the patch series for correct checking of read_allocated_blocks 
return values and fix all the other block number checks.

Anyway, it would be good to know why *exactly* read_allocated_blocks returns 
an error code in your case. Do you remember the exact negative value returned 
(there are -EINVAL and -ENOMEN, and many several unspecific uses of 0 and -1). 

Can you provide a disk image of the failing file system?

Kind regards,

Stefan

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

[Thomas Schaefer]

> -----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,
> > 
> > U-boots ext4 implementation currently does not support 64bit or even 
> > 48bit block numbers, so this may be the cause.
> > 
> > Can you provide some information about your test setup?
> > 
> > You can use the debugsfs ext tool to gather some information about the 
> > problematic file. Just access the filesystem with:
> > 
> > /sbin/debugfs /dev/sda1  ;  (or whatever your partion name is)
> > 
> >   or
> > 
> > /sbin/debugfs /path/to/imagefile
> > 
> > debugfs supports commands like cd, stat, ls. stat gives you the block 
> > number list.
> > 
> > Kind regards,
> > 
> > Stefan
>
> 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!
>
>
> I will update the patch series for correct checking of read_allocated_blocks return values and fix all the other block number checks.
>
> Anyway, it would be good to know why *exactly* read_allocated_blocks returns an error code in your case. Do you remember the exact negative value returned (there are -EINVAL and -> > > > > ENOMEN, and many several unspecific uses of 0 and -1). 
>
> Can you provide a disk image of the failing file system?
>
> Kind regards,
>
> Stefan

Hi Stefan,

the attachment contains an image file that causes u-boot to crash when trying to overwrite existing files in ext4 fs.

Calling debugfs show the following output

root at s1909:~# /sbin/debugfs /dev/mmcblk0p1
debugfs 1.43-WIP (18-May-2015)
debugfs:  ls
 2  (12) .    2  (12) ..    11  (20) lost+found    12  (12) foo
 7729  (4040) images
debugfs:  ls foo
 12  (12) .    2  (12) ..    13  (16) test.bin    14  (4056) test2.bin
debugfs:  stat foo/test.bin
Inode: 13   Type: regular    Mode:  0644   Flags: 0x80000
Generation: 2312001584    Version: 0x00000000:00000001
User:     0   Group:     0   Size: 559364
File ACL: 0    Directory ACL: 0
Links: 1   Blockcount: 1096
Fragment:  Address: 0    Number: 0    Size: 0
 ctime: 0x57c93d53:3a2e2ac0 -- Fri Sep  2 08:50:27 2016
 atime: 0x57c98a2b:00001708 -- Fri Sep  2 14:18:19 2016
 mtime: 0x57c93d53:3a2e2ac0 -- Fri Sep  2 08:50:27 2016
crtime: 0x57c93d53:243eee64 -- Fri Sep  2 08:50:27 2016
Size of extra inode fields: 32
EXTENTS:
(0-136):32852-32988
debugfs:  stat foo/test2.bin
Inode: 14   Type: regular    Mode:  0644   Flags: 0x80000
Generation: 2312001585    Version: 0x00000000:00000001
User:     0   Group:     0   Size: 559364
File ACL: 0    Directory ACL: 0
Links: 1   Blockcount: 1096
Fragment:  Address: 0    Number: 0    Size: 0
 ctime: 0x57c93d5d:e206ed50 -- Fri Sep  2 08:50:37 2016
 atime: 0x57c93d5d:cc17b0f4 -- Fri Sep  2 08:50:37 2016
 mtime: 0x57c93d5d:e206ed50 -- Fri Sep  2 08:50:37 2016
crtime: 0x57c93d5d:cc17b0f4 -- Fri Sep  2 08:50:37 2016
Size of extra inode fields: 32
EXTENTS:
(0-136):32989-33125
debugfs: q


Thank you and best regards,
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mmcp1.img.gz
Type: application/x-gzip
Size: 2963562 bytes
Desc: mmcp1.img.gz
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160902/6c171d09/attachment-0001.bin>

[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