On Fri, Apr 21, 2023 at 1:06 AM Chao Yu <c...@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 <akoskov...@pm.me>
> > Signed-off-by: Juhyung Park <qkrwngud...@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

Reply via email to