Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni <xni@redhat.com>]

On Mon, Oct 18, 2021 at 2:56 PM Tkaczyk, Mariusz
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Xiao,
> Thanks for taking care of this.
>
> On 15.10.2021 11:25, Xiao Ni wrote:
> > The test case is:
> > 1. create one imsm container
> > 2. create a raid5 device from the container
> > 3. unplug two disks
> > 4. mdadm --detail /dev/md126
> > [root@rhel85 ~]# mdadm -D /dev/md126
> > /dev/md126:
> >           Container : ��, member 0
> >
> > The Detail function first gets container name by function
> > map_dev_preferred. Then it tries to find which disks are
> > available. In patch db5377883fef(It should be FAILED..)
> > uses map_dev_preferred to find which disks are under /dev.
> >
> > But now, the major/minor information comes from kernel space.
> > map_dev_preferred malloc memory and init a device list when
> > first be called by Detail. It can't find the device in the
> > list by the major/minor. It free the memory and reinit the
> > list.
> >  > The container name now points to an area tha has been freed.
> > So the containt is a mess.
> >
>
> Container name is collected with 'create' flag set, so it's
> name is additionally copied to static memory to prevent
> overwrites. Could you verify?

Hi Mariusz

The chapter above you mentioned is talking about the creation process?
The container name mentioned from the patch is a temporary variable in
Detail function.

You want to say we can use the container name from the "static memory" in
Detail function, so we don't get the container name again? And where is the
static memory?

>
> So summarizing: the previously returned value might be lost
> in next call.

If you say the value returned from map_dev_preffered, you are right,
the buffer might be free and re-alloc. So the value might be lost
in the next call.

>
> I looked into code, map_dev_preffered() mainly is used for
> determining short-life buffers, which are used only to the next
> call (next call overwrites previous result, expect case you fixed).

right

>
> IMO map_dev_preffered() cannot be trusted now. I see some options:
> 1 - allocate additional memory and save return value there (caller
> needs to free it later).
> 2 - describe limitations in description of the function to avoid
> incorrect usages in the future.

I'll add one description first.

>
> > This patch replaces map_dev_preferred with devid2kname. If
> > the name is NULL, it means the disk is unplug.
> >
> Your patch fixes only one place. Please go forward and analyze all
> map_dev_preffered() calls (which looks safe to me). Maybe this
> function can be replaced at all and we can drop this code in
> flavor of devid2kname() or other.

At first, I just wanted to fix this bug. We can check all the places which
are using map_dev_preffered. But it looks like a big project. It needs to
understand all codes related to this function. It needs more time. Do you
agree with fixing this bug first, then we can try to fix the hidden bugs.

>
> > Fixes: db5377883fef (It should be FAILED when raid has)
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > Reported-by: Fine Fan <ffan@redhat.com>
> > ---
> >   Detail.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/Detail.c b/Detail.c
> > index d3af0ab..2164de3 100644
> > --- a/Detail.c
> > +++ b/Detail.c
> > @@ -351,11 +351,13 @@ int Detail(char *dev, struct context *c)
> >       avail = xcalloc(array.raid_disks, 1);
> >
> >       for (d = 0; d < array.raid_disks; d++) {
> > -             char *dv, *dv_rep;
> > -             dv = map_dev_preferred(disks[d*2].major,
> > -                             disks[d*2].minor, 0, c->prefer);
> > -             dv_rep = map_dev_preferred(disks[d*2+1].major,
> > -                             disks[d*2+1].minor, 0, c->prefer);
> > +             char *dv, *dv_rep = NULL;
> > +
> > +             if (!disks[d*2].major && !disks[d*2].minor)
> > +                     continue; > +
> > +             dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
> > +             dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
> >
> >               if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
> >                   (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
> >
>
> Yeah, I know that it is used in Detail this way, but please  determine
> way to replace this ugly [d*2] and [d*2+1].

Yes, it takes me much time to understand how to calculate avail[]. We
can focus on
fixing this bug first, then we prepare some patches for improving the codes that
related to the function map_dev_preferred and the [d*2] format codes.
It might be
a big change.

>
> This whole block should be moved from Detail() code to separate
> function, which determines if device or replacement is in sync.

A good suggestion. Put it into the change I mentioned above, is it ok?

Regards
Xiao