| Message ID | 20230404055446.1656296-1-qkrwngud825@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] fsck.f2fs: fix sanity check logic for cp_payload | expand |
Hi Jaegeuk, Thanks for the timely merge. Can we have a new f2fs-tools tag with this commit in place? Alex (the original bug reporter) would like to see this incorporated in the next version of Ubuntu (and Debian), and having a new tag is much easier to convince them to pack a new tag than having one patch backported. In a practical scenario, a 2TB partition formatted with mkfs.f2fs -i simply doesn't boot under Ubuntu as fsck.f2fs inside the initramfs returns an error. This patch fixes that and therefore allows the users to boot normally without manually excluding the f2fs partition to be checked during boot. Thanks. Regards On Tue, Apr 4, 2023 at 2:54 PM Juhyung Park <qkrwngud825@gmail.com> wrote: > > cp_payload is set differently [1] when extended node bitmap feature is > enabled. Commit b79c3ba4ea9d broke fsck on f2fs file systems created on > 2+ TB device with extended node bitmap feature enabled. > > As the sanity check is for checking overflows, fix this to assume the max > possible cp_payload size under the extended node bitmap. > > Link: https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/tree/mkfs/f2fs_format.c?h=v1.15.0#n372 [1] > Fixes: b79c3ba4ea9d ("fsck.f2fs: sanity check cp_payload before reading checkpoint") > Reported-by: Alexander Koskovich <akoskovich@pm.me> > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> > --- > fsck/mount.c | 2 +- > include/f2fs_fs.h | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fsck/mount.c b/fsck/mount.c > index 2b26701..df0314d 100644 > --- a/fsck/mount.c > +++ b/fsck/mount.c > @@ -1208,7 +1208,7 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) > int ret; > > cp_payload = get_sb(cp_payload); > - if (cp_payload > F2FS_BLK_ALIGN(MAX_SIT_BITMAP_SIZE)) > + if (cp_payload > F2FS_BLK_ALIGN(MAX_CP_PAYLOAD)) > return -EINVAL; > > cp_blks = 1 + cp_payload; > diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h > index 333ae07..f890634 100644 > --- a/include/f2fs_fs.h > +++ b/include/f2fs_fs.h > @@ -1168,6 +1168,10 @@ static_assert(sizeof(struct f2fs_nat_block) == 4095, ""); > #define MAX_SIT_BITMAP_SIZE (SEG_ALIGN(SIZE_ALIGN(F2FS_MAX_SEGMENT, \ > SIT_ENTRY_PER_BLOCK)) * \ > c.blks_per_seg / 8) > +#define MAX_CP_PAYLOAD (SEG_ALIGN(SIZE_ALIGN(UINT32_MAX, NAT_ENTRY_PER_BLOCK)) * \ > + DEFAULT_NAT_ENTRY_RATIO / 100 * \ > + c.blks_per_seg / 8 + \ > + MAX_SIT_BITMAP_SIZE - MAX_BITMAP_SIZE_IN_CKPT) > > /* > * Note that f2fs_sit_entry->vblocks has the following bit-field information. > -- > 2.40.0 >
On 04/10, Juhyung Park wrote: > Hi Jaegeuk, > > Thanks for the timely merge. > > Can we have a new f2fs-tools tag with this commit in place? > Alex (the original bug reporter) would like to see this incorporated > in the next version of Ubuntu (and Debian), and having a new tag is > much easier to convince them to pack a new tag than having one patch > backported. > > In a practical scenario, a 2TB partition formatted with mkfs.f2fs -i > simply doesn't boot under Ubuntu as fsck.f2fs inside the initramfs > returns an error. > > This patch fixes that and therefore allows the users to boot normally > without manually excluding the f2fs partition to be checked during > boot. Thank you for heads up. I released v1.16.0. Please check. :) https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/tag/?h=v1.16.0 > > Thanks. Regards > > On Tue, Apr 4, 2023 at 2:54 PM Juhyung Park <qkrwngud825@gmail.com> wrote: > > > > cp_payload is set differently [1] when extended node bitmap feature is > > enabled. Commit b79c3ba4ea9d broke fsck on f2fs file systems created on > > 2+ TB device with extended node bitmap feature enabled. > > > > As the sanity check is for checking overflows, fix this to assume the max > > possible cp_payload size under the extended node bitmap. > > > > Link: https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/tree/mkfs/f2fs_format.c?h=v1.15.0#n372 [1] > > Fixes: b79c3ba4ea9d ("fsck.f2fs: sanity check cp_payload before reading checkpoint") > > Reported-by: Alexander Koskovich <akoskovich@pm.me> > > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> > > --- > > fsck/mount.c | 2 +- > > include/f2fs_fs.h | 4 ++++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/fsck/mount.c b/fsck/mount.c > > index 2b26701..df0314d 100644 > > --- a/fsck/mount.c > > +++ b/fsck/mount.c > > @@ -1208,7 +1208,7 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) > > int ret; > > > > cp_payload = get_sb(cp_payload); > > - if (cp_payload > F2FS_BLK_ALIGN(MAX_SIT_BITMAP_SIZE)) > > + if (cp_payload > F2FS_BLK_ALIGN(MAX_CP_PAYLOAD)) > > return -EINVAL; > > > > cp_blks = 1 + cp_payload; > > diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h > > index 333ae07..f890634 100644 > > --- a/include/f2fs_fs.h > > +++ b/include/f2fs_fs.h > > @@ -1168,6 +1168,10 @@ static_assert(sizeof(struct f2fs_nat_block) == 4095, ""); > > #define MAX_SIT_BITMAP_SIZE (SEG_ALIGN(SIZE_ALIGN(F2FS_MAX_SEGMENT, \ > > SIT_ENTRY_PER_BLOCK)) * \ > > c.blks_per_seg / 8) > > +#define MAX_CP_PAYLOAD (SEG_ALIGN(SIZE_ALIGN(UINT32_MAX, NAT_ENTRY_PER_BLOCK)) * \ > > + DEFAULT_NAT_ENTRY_RATIO / 100 * \ > > + c.blks_per_seg / 8 + \ > > + MAX_SIT_BITMAP_SIZE - MAX_BITMAP_SIZE_IN_CKPT) > > > > /* > > * Note that f2fs_sit_entry->vblocks has the following bit-field information. > > -- > > 2.40.0 > >
On 2023/4/4 13:54, Juhyung Park wrote: > cp_payload is set differently [1] when extended node bitmap feature is > enabled. Commit b79c3ba4ea9d broke fsck on f2fs file systems created on > 2+ TB device with extended node bitmap feature enabled. > > As the sanity check is for checking overflows, fix this to assume the max > possible cp_payload size under the extended node bitmap. > > Link: https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/tree/mkfs/f2fs_format.c?h=v1.15.0#n372 [1] > Fixes: b79c3ba4ea9d ("fsck.f2fs: sanity check cp_payload before reading checkpoint") > Reported-by: Alexander Koskovich <akoskovich@pm.me> > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> > --- > fsck/mount.c | 2 +- > include/f2fs_fs.h | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fsck/mount.c b/fsck/mount.c > index 2b26701..df0314d 100644 > --- a/fsck/mount.c > +++ b/fsck/mount.c > @@ -1208,7 +1208,7 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) > int ret; > > cp_payload = get_sb(cp_payload); > - if (cp_payload > F2FS_BLK_ALIGN(MAX_SIT_BITMAP_SIZE)) > + if (cp_payload > F2FS_BLK_ALIGN(MAX_CP_PAYLOAD)) > return -EINVAL; > > cp_blks = 1 + cp_payload; > diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h > index 333ae07..f890634 100644 > --- a/include/f2fs_fs.h > +++ b/include/f2fs_fs.h > @@ -1168,6 +1168,10 @@ static_assert(sizeof(struct f2fs_nat_block) == 4095, ""); > #define MAX_SIT_BITMAP_SIZE (SEG_ALIGN(SIZE_ALIGN(F2FS_MAX_SEGMENT, \ > SIT_ENTRY_PER_BLOCK)) * \ > c.blks_per_seg / 8) > +#define MAX_CP_PAYLOAD (SEG_ALIGN(SIZE_ALIGN(UINT32_MAX, NAT_ENTRY_PER_BLOCK)) * \ > + DEFAULT_NAT_ENTRY_RATIO / 100 * \ DEFAULT_NAT_ENTRY_RATIO is 20 now, if we change it to 100 later, old fsck will still report error on a new image? Should we remove "DEFAULT_NAT_ENTRY_RATIO / 100"? Thanks, > + c.blks_per_seg / 8 + \ > + MAX_SIT_BITMAP_SIZE - MAX_BITMAP_SIZE_IN_CKPT) > > /* > * Note that f2fs_sit_entry->vblocks has the following bit-field information.
On Fri, Apr 21, 2023 at 1:06 AM Chao Yu <chao@kernel.org> wrote: > > On 2023/4/4 13:54, Juhyung Park wrote: > > cp_payload is set differently [1] when extended node bitmap feature is > > enabled. Commit b79c3ba4ea9d broke fsck on f2fs file systems created on > > 2+ TB device with extended node bitmap feature enabled. > > > > As the sanity check is for checking overflows, fix this to assume the max > > possible cp_payload size under the extended node bitmap. > > > > Link: https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/tree/mkfs/f2fs_format.c?h=v1.15.0#n372 [1] > > Fixes: b79c3ba4ea9d ("fsck.f2fs: sanity check cp_payload before reading checkpoint") > > Reported-by: Alexander Koskovich <akoskovich@pm.me> > > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> > > --- > > fsck/mount.c | 2 +- > > include/f2fs_fs.h | 4 ++++ > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/fsck/mount.c b/fsck/mount.c > > index 2b26701..df0314d 100644 > > --- a/fsck/mount.c > > +++ b/fsck/mount.c > > @@ -1208,7 +1208,7 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) > > int ret; > > > > cp_payload = get_sb(cp_payload); > > - if (cp_payload > F2FS_BLK_ALIGN(MAX_SIT_BITMAP_SIZE)) > > + if (cp_payload > F2FS_BLK_ALIGN(MAX_CP_PAYLOAD)) > > return -EINVAL; > > > > cp_blks = 1 + cp_payload; > > diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h > > index 333ae07..f890634 100644 > > --- a/include/f2fs_fs.h > > +++ b/include/f2fs_fs.h > > @@ -1168,6 +1168,10 @@ static_assert(sizeof(struct f2fs_nat_block) == 4095, ""); > > #define MAX_SIT_BITMAP_SIZE (SEG_ALIGN(SIZE_ALIGN(F2FS_MAX_SEGMENT, \ > > SIT_ENTRY_PER_BLOCK)) * \ > > c.blks_per_seg / 8) > > +#define MAX_CP_PAYLOAD (SEG_ALIGN(SIZE_ALIGN(UINT32_MAX, NAT_ENTRY_PER_BLOCK)) * \ > > + DEFAULT_NAT_ENTRY_RATIO / 100 * \ > > DEFAULT_NAT_ENTRY_RATIO is 20 now, if we change it to 100 later, > old fsck will still report error on a new image? > > Should we remove "DEFAULT_NAT_ENTRY_RATIO / 100"? > That's a valid concern, if we plan to increase it in the future. I wasn't sure how strict the overflow check must be done. Feel free to send a patch to increase this. I suppose your concern doesn't really have an immediate practical impact as this is set from mkfs and isn't user configurable. Thanks, > Thanks, > > > + c.blks_per_seg / 8 + \ > > + MAX_SIT_BITMAP_SIZE - MAX_BITMAP_SIZE_IN_CKPT) > > > > /* > > * Note that f2fs_sit_entry->vblocks has the following bit-field information.
diff --git a/fsck/mount.c b/fsck/mount.c index 2b26701..df0314d 100644 --- a/fsck/mount.c +++ b/fsck/mount.c @@ -1208,7 +1208,7 @@ int get_valid_checkpoint(struct f2fs_sb_info *sbi) int ret; cp_payload = get_sb(cp_payload); - if (cp_payload > F2FS_BLK_ALIGN(MAX_SIT_BITMAP_SIZE)) + if (cp_payload > F2FS_BLK_ALIGN(MAX_CP_PAYLOAD)) return -EINVAL; cp_blks = 1 + cp_payload; diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h index 333ae07..f890634 100644 --- a/include/f2fs_fs.h +++ b/include/f2fs_fs.h @@ -1168,6 +1168,10 @@ static_assert(sizeof(struct f2fs_nat_block) == 4095, ""); #define MAX_SIT_BITMAP_SIZE (SEG_ALIGN(SIZE_ALIGN(F2FS_MAX_SEGMENT, \ SIT_ENTRY_PER_BLOCK)) * \ c.blks_per_seg / 8) +#define MAX_CP_PAYLOAD (SEG_ALIGN(SIZE_ALIGN(UINT32_MAX, NAT_ENTRY_PER_BLOCK)) * \ + DEFAULT_NAT_ENTRY_RATIO / 100 * \ + c.blks_per_seg / 8 + \ + MAX_SIT_BITMAP_SIZE - MAX_BITMAP_SIZE_IN_CKPT) /* * Note that f2fs_sit_entry->vblocks has the following bit-field information.
cp_payload is set differently [1] when extended node bitmap feature is enabled. Commit b79c3ba4ea9d broke fsck on f2fs file systems created on 2+ TB device with extended node bitmap feature enabled. As the sanity check is for checking overflows, fix this to assume the max possible cp_payload size under the extended node bitmap. Link: https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/tree/mkfs/f2fs_format.c?h=v1.15.0#n372 [1] Fixes: b79c3ba4ea9d ("fsck.f2fs: sanity check cp_payload before reading checkpoint") Reported-by: Alexander Koskovich <akoskovich@pm.me> Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> --- fsck/mount.c | 2 +- include/f2fs_fs.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)