RE: problem with exfat on 4k logical sector devices

["Namjae Jeon" <namjae.jeon@samsung.com>]

> 
> On 5/12/21 9:09 AM, Hyunchul Lee wrote:
> > Hello,
> >
> > 2021년 5월 12일 (수) 오전 8:57, Eric Sandeen <sandeen@sandeen.net>님이 작성:
> >>
> >> On 5/11/21 6:53 PM, Namjae Jeon wrote:
> >>
> >>>> One other thing that I ran across is that fsck seems to validate an
> >>>> image against the sector size of the device hosting the image
> >>>> rather than the sector size found in the boot sector, which seems like another issue that will
> come up:
> >>>>
> >>>> # fsck/fsck.exfat /dev/sdb
> >>>> exfatprogs version : 1.1.1
> >>>> /dev/sdb: clean. directories 1, files 0
> >>>>
> >>>> # dd if=/dev/sdb of=test.img
> >>>> 524288+0 records in
> >>>> 524288+0 records out
> >>>> 268435456 bytes (268 MB) copied, 1.27619 s, 210 MB/s
> >>>>
> >>>> # fsck.exfat test.img
> >>>> exfatprogs version : 1.1.1
> >>>> checksum of boot region is not correct. 0, but expected 0x3ee721
> >>>> boot region is corrupted. try to restore the region from backup.
> >>>> Fix (y/N)? n
> >>>>
> >>>> Right now the utilities seem to assume that the device they're
> >>>> pointed at is always a block device, and image files are problematic.
> >>> Okay, Will fix it.
> >>
> >> Right now I have a hack like this.
> >>
> >> 1) don't validate the in-image sector size against the host device
> >> size (maybe should only skip this check if it's not a bdev? Or is it
> >> OK to have a 4k sector size fs on a 512 device? Probably?)
> >>
> >> 2) populate the "bd" sector size information from the values read from the image.
> >>
> >> It feels a bit messy, but it works so far. I guess the messiness
> >> stems from assuming that we always have a "bd" block device.
> >>
> >
> > I think we need to keep the "bd" sector size to avoid confusion
> > between the device's sector size and the exfat's sector size.
> 
> Sure, it's just that for a filesystem in an image file, there is no meaning to the "device sector
> size" because there is no device.
> 
> ...
> 
> > Is it better to add a sector size parameter to read_boot_region
> > function? This function is also called to read the backup boot region
> > to restore the corrupted main boot region.
> > During this restoration, we need to read the backup boot region with
> > various sector sizes, Because we don't have a certain exfat sector
> > size.
> >
> >>         ret = boot_region_checksum(bd, bs_offset);
> >>         if (ret < 0)
> >>                 goto err;
> >>
> >>
> >
> > I sent the pull request to fix these problems. Could you check this request?
> > https://protect2.fireeye.com/v1/url?k=7932f7a1-26a9cef7-79337cee-0cc47
> > a31ce52-924b76d62e7bfc04&q=1&e=433c5d9e-f62a-4378-9b98-3c965d70e4da&u=
> > https%3A%2F%2Fgithub.com%2Fexfatprogs%2Fexfatprogs%2Fpull%2F167
> 
> I didn't review that in depth, but it looks like for fsck and dump, it gets the sector size from the
> boot sector rather than from the host device or filesystem, which makes sense, at least.
> 
> (As an aside, I'd suggest that your new "c2o" function could have a more descriptive, self-documenting
> name.)
> 
> But there are other problems where bd->sector_* is used and assumed to relate to the filesystem, for
> example:
> 
> # fsck/fsck.exfat /root/test.img
> exfatprogs version : 1.1.1
> /root/test.img: clean. directories 1, files 0 # tune/tune.exfat -I 0x1234  /root/test.img exfatprogs
> version : 1.1.1 New volume serial : 0x1234 # fsck/fsck.exfat /root/test.img exfatprogs version : 1.1.1
> checksum of boot region is not correct. 0x3eedc5, but expected 0xe59577e3 boot region is corrupted.
> try to restore the region from backup. Fix (y/N)? n
> 
> I think because exfat_write_checksum_sector() relies on bd->sector_size.
> 
> You probably need to audit every use of bd->sector_size and
> bd->sector_size_bits outside of mkfs, because anything assuming that it
> is related to the filesystem itself, as opposed to the filesytem/device hosting it, could be
> problematic.  Is there any time outside of mkfs that you actually care about the device sector size?
> If not, I might suggest trying to isolate that usage to mkfs.
I do not object to updating bd->sector_size to sector size obtained from boot sector.
I think that's the best way to do it.

> 
> I also wonder about:
> 
>         bd->num_sectors = blk_dev_size / DEFAULT_SECTOR_SIZE;
>         bd->num_clusters = blk_dev_size / ui->cluster_size;
> 
> is it really correct that this should always be in terms of 512-byte sectors?
Yep, Need to fix it.

Thanks!
> 
> -Eric