Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230328094400.1448955-1-yukuai1@huaweicloud.com>
In-Reply-To: <20230328094400.1448955-1-yukuai1@huaweicloud.com>
From:   Song Liu <song@kernel.org>
Date:   Tue, 28 Mar 2023 14:42:48 -0700
Message-ID: <CAPhsuW4hRBQ9YooAOHsZhio88ykAtrbxxZBfKND_5CzhJBHQoQ@mail.gmail.com>
Subject: Re: [PATCH -next] md: fix regression for null-ptr-deference in __md_stop()
To:     Yu Kuai <yukuai1@huaweicloud.com>
Cc:     xni@redhat.com, logang@deltatee.com, linux-raid@vger.kernel.org,
        linux-kernel@vger.kernel.org, yukuai3@huawei.com,
        yi.zhang@huawei.com, yangerkun@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 28, 2023 at 2:44=E2=80=AFAM Yu Kuai <yukuai1@huaweicloud.com> w=
rote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> Commit 3e453522593d ("md: Free resources in __md_stop") tried to fix
> null-ptr-deference for 'active_io' by moving percpu_ref_exit() to
> __md_stop(), however, the commit also moving 'writes_pending' to
> __md_stop(), and this will cause mdadm tests broken:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000038
> Oops: 0000 [#1] PREEMPT SMP
> CPU: 15 PID: 17830 Comm: mdadm Not tainted 6.3.0-rc3-next-20230324-00009-=
g520d37
> RIP: 0010:free_percpu+0x465/0x670
> Call Trace:
>  <TASK>
>  __percpu_ref_exit+0x48/0x70
>  percpu_ref_exit+0x1a/0x90
>  __md_stop+0xe9/0x170
>  do_md_stop+0x1e1/0x7b0
>  md_ioctl+0x90c/0x1aa0
>  blkdev_ioctl+0x19b/0x400
>  vfs_ioctl+0x20/0x50
>  __x64_sys_ioctl+0xba/0xe0
>  do_syscall_64+0x6c/0xe0
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> And the problem can be reporduced 100% by following test:
>
> mdadm -CR /dev/md0 -l1 -n1 /dev/sda --force
> echo inactive > /sys/block/md0/md/array_state
> echo read-auto  > /sys/block/md0/md/array_state
> echo inactive > /sys/block/md0/md/array_state
>
> Root cause:
>
> // start raid
> raid1_run
>  mddev_init_writes_pending
>   percpu_ref_init
>
> // inactive raid
> array_state_store
>  do_md_stop
>   __md_stop
>    percpu_ref_exit
>
> // start raid again
> array_state_store
>  do_md_run
>   raid1_run
>    mddev_init_writes_pending
>     if (mddev->writes_pending.percpu_count_ptr)
>     // won't reinit
>
> // inactive raid again
> ...
> percpu_ref_exit
> -> null-ptr-deference
>
> Before the commit, 'writes_pending' is exited when mddev is freed, and
> it's safe to restart raid because mddev_init_writes_pending() already mak=
e
> sure that 'writes_pending' will only be initialized once.
>
> Fix the prblem by moving 'writes_pending' back, it's a litter hard to fin=
d
> the relationship between alloc memory and free memory, however, code
> changes is much less and we lived with this for a long time already.
>
> Fixes: 3e453522593d ("md: Free resources in __md_stop")
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Applied to md-fixes.

Thanks!
Song