* [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload
@ 2023-04-04 5:54 Juhyung Park
2023-04-10 8:44 ` Juhyung Park
2023-04-20 16:06 ` Chao Yu
0 siblings, 2 replies; 5+ messages in thread
From: Juhyung Park @ 2023-04-04 5:54 UTC (permalink / raw)
To: linux-f2fs-devel; +Cc: Alexander Koskovich
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload
2023-04-04 5:54 [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload Juhyung Park
@ 2023-04-10 8:44 ` Juhyung Park
2023-04-11 17:17 ` Jaegeuk Kim
2023-04-20 16:06 ` Chao Yu
1 sibling, 1 reply; 5+ messages in thread
From: Juhyung Park @ 2023-04-10 8:44 UTC (permalink / raw)
To: Jaegeuk Kim, linux-f2fs-devel; +Cc: Alexander Koskovich
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
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload
2023-04-10 8:44 ` Juhyung Park
@ 2023-04-11 17:17 ` Jaegeuk Kim
0 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2023-04-11 17:17 UTC (permalink / raw)
To: Juhyung Park; +Cc: Alexander Koskovich, linux-f2fs-devel
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
> >
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload
2023-04-04 5:54 [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload Juhyung Park
2023-04-10 8:44 ` Juhyung Park
@ 2023-04-20 16:06 ` Chao Yu
2023-04-20 17:02 ` Juhyung Park
1 sibling, 1 reply; 5+ messages in thread
From: Chao Yu @ 2023-04-20 16:06 UTC (permalink / raw)
To: Juhyung Park, linux-f2fs-devel; +Cc: Alexander Koskovich
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.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload
2023-04-20 16:06 ` Chao Yu
@ 2023-04-20 17:02 ` Juhyung Park
0 siblings, 0 replies; 5+ messages in thread
From: Juhyung Park @ 2023-04-20 17:02 UTC (permalink / raw)
To: Chao Yu; +Cc: Alexander Koskovich, linux-f2fs-devel
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.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-20 17:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-04 5:54 [f2fs-dev] [PATCH] fsck.f2fs: fix sanity check logic for cp_payload Juhyung Park
2023-04-10 8:44 ` Juhyung Park
2023-04-11 17:17 ` Jaegeuk Kim
2023-04-20 16:06 ` Chao Yu
2023-04-20 17:02 ` Juhyung Park
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.