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

On Mon, Oct 18, 2021 at 2:56 PM Tkaczyk, Mariusz
[off-list ref] wrote:

Hi Xiao,
Thanks for taking care of this.

On 15.10.2021 11:25, Xiao Ni wrote:

quoted

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.

quoted

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.

quoted

Fixes: db5377883fef (It should be FAILED when raid has)
Signed-off-by: Xiao Ni <redacted>
Reported-by: Fine Fan <redacted>
---
  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