linux-kernel - Re: [PATCH] bitmap: fix a unproper remap when mpol_rebind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Jun 2022 12:20:26 -0700
From:   Yury Norov <yury.norov@...il.com>
To:     Yi Wang <wang.yi59@....com.cn>
Cc:     andriy.shevchenko@...ux.intel.com, linux@...musvillemoes.dk,
        linux-kernel@...r.kernel.org, xue.zhihong@....com.cn,
        wang.liang82@....com.cn
Subject: Re: [PATCH] bitmap: fix a unproper remap when mpol_rebind_nodemask()

On Mon, Jun 13, 2022 at 4:31 AM Yi Wang <wang.yi59@....com.cn> wrote:
>
> Consider one situation:
>
> The app have two vmas which mbind() to node 1 and node3 respectively,
> and its cpuset.mems is 0-3, now set its cpuset.mems to 1,3, according
> to current bitmap_remap(), we got:
>
>     1 => 3
>     3 => 3
>
> This maybe confused because node 1,3 have already in the new settiing
> region but both nodes are binded to the same node 3 now.
>
> Actually we found the situation on a very old libvirt and qemu, but
> this can be easily reproduced in the current kernel, so we try to fix
> it.
>
> A possible fix way is to ignore the bits in @src have already existed
> in @new.
>
> Signed-off-by: Yi Wang <wang.yi59@....com.cn>
> ---
>  lib/bitmap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index b18e31ea6e66..b77bf1b3852e 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1006,8 +1006,8 @@ unsigned int bitmap_ord_to_pos(const unsigned long *buf, unsigned int ord, unsig
>   * @dst point to the same location, then this routine copies @src
>   * to @dst.
>   *
> - * The positions of unset bits in @old are mapped to themselves
> - * (the identify map).
> + * The positions of unset bits in @old or bits in @src have already
> + * existed in @new are mapped to themselves (the identify map).
>   *
>   * Apply the above specified mapping to @src, placing the result in
>   * @dst, clearing any bits previously set in @dst.
> @@ -1033,7 +1033,7 @@ void bitmap_remap(unsigned long *dst, const unsigned long *src,
>         for_each_set_bit(oldbit, src, nbits) {
>                 int n = bitmap_pos_to_ord(old, oldbit, nbits);
>
> -               if (n < 0 || w == 0)
> +               if (n < 0 || w == 0 || test_bit(oldbit, new))
>                         set_bit(oldbit, dst);   /* identity map */
>                 else
>                         set_bit(bitmap_ord_to_pos(new, n % w, nbits), dst);
> --
> 2.33.0.rc0.dirty

Regarding the original problem - can you please confirm that
it's reproduced on current kernels, show the execution path etc.
>From what I see on modern kernel, the only user of nodes_remap()
is mpol_rebind_nodemask(). Is that the correct path?

Anyways, as per name, bitmap_remap() is intended to change bit
positions, and it doesn't look wrong if it does so.

This is not how the function is supposed to work. For example,
        old: 00111000
        new: 00011100

means:
        old: 00111 000
             || \\\|||
        new: 000 11100

And after this patch it would be:
        old: 001 11000
             || \|||||
        new: 000 11100

Which is not the same, right?

If mpol_rebind() wants to keep previous relations, then according to
the comment:
 * The positions of unset bits in @old are mapped to themselves
 * (the identify map).

, you can just clear @old bits that already have good relations
you'd like to preserve.

Thanks,
Yury