Re: [PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store

On Thu, Apr 16, 2026 at 10:09 PM Su Yue <[email protected]> wrote:
>
> On Wed 15 Apr 2026 at 18:34, Xiao Ni <[email protected]> wrote:
>
> > On Tue, Apr 7, 2026 at 6:26 PM Su Yue <[email protected]> wrote:
> >>
> >> If bitmap/location is present, mdadm will call
> >> update_array_info()
> >> while growing bitmap from none to internal via
> >> location_store().
> >> md_bitmap_create() is needed to set mddev->bitmap_ops otherwise
> >> mddev->bitmap_ops->get_stats() in update_array_info() will
> >> trigger
> >> kernel NULL pointer dereference.
> >
> >
> > Hi Su Yue
> >
> > How can bitmap/location be present when bitmap is none? Could
> > you
> > provide the test commands that reproduce this problem?
> >
> Sorry for the misleading commit message. It can only be reproduced
> patch 3 is appiled.
> I adjusted the sequence of this patch for easy review because
> md_bitmap_create,destroy
> are touched in patch1,2 and 3. Also if put the patch after 3rd
> patch,
> it will break ability to bisect.
>
> # mdadm --create --assume-clean /dev/md0 -f --bitmap=internal
>   --raid-devices=2 --level=mirror --metadata=1.2 /dev/vdc /dev/vdd
> # mdadm --grow /dev/md0 --bitmap=none
> # mdadm --grow /dev/md0 --bitmap=internal # step 3
> # mdadm --grow /dev/md0 --bitmap=none # step 4
> [1]    2325 killed     mdadm --grow /dev/md0 --bitmap=none
>
> When step 3 is called,
> md_bitmap_destroy() is called in update_array_info() to set NULL
> mddev->bitmap_ops
> then in step 4 kernel Oops is triggered.
>
>
> I am willing to amend commit message or move it after patch 3 if
> you would like.

Hi Su

Thanks for the detail explanation. After reading patch3, I totoally
understand. The sequence is good to me. And yes, it's better to
explain that this is needed after patch3.

Best Regards
Xiao
>
> --
> Su
>
> >
> > mdadm -CR /dev/md0 -l1 -n2 /dev/loop0 /dev/loop1 --bitmap=none
> > (There
> > is not bitmap/location, because bitmap directory is not created)
> > mdadm /dev/md0 --grow --bitmap=internal
> > Grow.c md_set_array_info runs
> >  451             array.state |= (1 << MD_SB_BITMAP_PRESENT);
> >  452             rv = md_set_array_info(fd, &array);
> > In kernel space, it runs
> >  8125             rv = md_bitmap_create(mddev);
> >  8126             if (!rv)
> >  8127                 rv = mddev->bitmap_ops->load(mddev);
> >
> > Best Regards
> > Xiao
> >
> >>
> >> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of
> >> bitmap_ops until creating bitmap")
> >> Signed-off-by: Su Yue <[email protected]>
> >> ---
> >>  drivers/md/md-bitmap.c | 11 ++++++++---
> >>  drivers/md/md.c        |  4 ++--
> >>  drivers/md/md.h        |  2 ++
> >>  3 files changed, 12 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> >> index 83378c033c72..2f24aae05552 100644
> >> --- a/drivers/md/md-bitmap.c
> >> +++ b/drivers/md/md-bitmap.c
> >> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const
> >> char *buf, size_t len)
> >>                         goto out;
> >>                 }
> >>
> >> -               bitmap_destroy(mddev);
> >> +               md_bitmap_destroy(mddev);
> >>                 mddev->bitmap_info.offset = 0;
> >>                 if (mddev->bitmap_info.file) {
> >>                         struct file *f =
> >>                         mddev->bitmap_info.file;
> >> @@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev,
> >> const char *buf, size_t len)
> >>                                 goto out;
> >>                         }
> >>
> >> +                       /*
> >> +                        * lockless bitmap shoudle have set
> >> bitmap_id
> >> +                        * using bitmap_type, so always
> >> ID_BITMAP.
> >> +                        */
> >> +                       mddev->bitmap_id = ID_BITMAP;
> >>                         mddev->bitmap_info.offset = offset;
> >> -                       rv = bitmap_create(mddev);
> >> +                       rv = md_bitmap_create(mddev);
> >>                         if (rv)
> >>                                 goto out;
> >>
> >>                         rv = bitmap_load(mddev);
> >>                         if (rv) {
> >>                                 mddev->bitmap_info.offset = 0;
> >> -                               bitmap_destroy(mddev);
> >> +                               md_bitmap_destroy(mddev);
> >>                                 goto out;
> >>                         }
> >>                 }
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 3ce6f9e9d38e..8b1ecc370ad6 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -6447,7 +6447,7 @@ static void md_safemode_timeout(struct
> >> timer_list *t)
> >>
> >>  static int start_dirty_degraded;
> >>
> >> -static int md_bitmap_create(struct mddev *mddev)
> >> +int md_bitmap_create(struct mddev *mddev)
> >>  {
> >>         if (mddev->bitmap_id == ID_BITMAP_NONE)
> >>                 return -EINVAL;
> >> @@ -6458,7 +6458,7 @@ static int md_bitmap_create(struct mddev
> >> *mddev)
> >>         return mddev->bitmap_ops->create(mddev);
> >>  }
> >>
> >> -static void md_bitmap_destroy(struct mddev *mddev)
> >> +void md_bitmap_destroy(struct mddev *mddev)
> >>  {
> >>         if (!md_bitmap_registered(mddev))
> >>                 return;
> >> diff --git a/drivers/md/md.h b/drivers/md/md.h
> >> index ac84289664cd..ed69244af00d 100644
> >> --- a/drivers/md/md.h
> >> +++ b/drivers/md/md.h
> >> @@ -895,6 +895,8 @@ static inline void safe_put_page(struct
> >> page *p)
> >>
> >>  int register_md_submodule(struct md_submodule_head *msh);
> >>  void unregister_md_submodule(struct md_submodule_head *msh);
> >> +int md_bitmap_create(struct mddev *mddev);
> >> +void md_bitmap_destroy(struct mddev *mddev);
> >>
> >>  extern struct md_thread *md_register_thread(
> >>         void (*run)(struct md_thread *thread),
> >> --
> >> 2.53.0
> >>
>