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;
Feedback-ID: i787e41f1:Fastmail
Date:   Wed, 27 Jul 2022 10:13:49 +0200
From:   Greg KH <greg@kroah.com>
To:     Yang Jihong <yangjihong1@huawei.com>
Cc:     peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
        alexander.shishkin@linux.intel.com, jolsa@redhat.com,
        namhyung@kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Subject: Re: [PATCH 4.19] perf/core: Fix data race between
 perf_event_set_output() and perf_mmap_close()
Message-ID: <YuDzvXC6vdnSwNHp@kroah.com>
References: <20220726013356.88395-1-yangjihong1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220726013356.88395-1-yangjihong1@huawei.com>
Precedence: bulk

On Tue, Jul 26, 2022 at 09:33:56AM +0800, Yang Jihong wrote:
> commit 68e3c69803dada336893640110cb87221bb01dcf upstream.
> 
> Yang Jihing reported a race between perf_event_set_output() and
> perf_mmap_close():
> 
>         CPU1                                    CPU2
> 
>         perf_mmap_close(e2)
>           if (atomic_dec_and_test(&e2->rb->mmap_count)) // 1 - > 0
>             detach_rest = true
> 
>                                                 ioctl(e1, IOC_SET_OUTPUT, e2)
>                                                   perf_event_set_output(e1, e2)
> 
>           ...
>           list_for_each_entry_rcu(e, &e2->rb->event_list, rb_entry)
>             ring_buffer_attach(e, NULL);
>             // e1 isn't yet added and
>             // therefore not detached
> 
>                                                     ring_buffer_attach(e1, e2->rb)
>                                                       list_add_rcu(&e1->rb_entry,
>                                                                    &e2->rb->event_list)
> 
> After this; e1 is attached to an unmapped rb and a subsequent
> perf_mmap() will loop forever more:
> 
>         again:
>                 mutex_lock(&e->mmap_mutex);
>                 if (event->rb) {
>                         ...
>                         if (!atomic_inc_not_zero(&e->rb->mmap_count)) {
>                                 ...
>                                 mutex_unlock(&e->mmap_mutex);
>                                 goto again;
>                         }
>                 }
> 
> The loop in perf_mmap_close() holds e2->mmap_mutex, while the attach
> in perf_event_set_output() holds e1->mmap_mutex. As such there is no
> serialization to avoid this race.
> 
> Change perf_event_set_output() to take both e1->mmap_mutex and
> e2->mmap_mutex to alleviate that problem. Additionally, have the loop
> in perf_mmap() detach the rb directly, this avoids having to wait for
> the concurrent perf_mmap_close() to get around to doing it to make
> progress.
> 
> Fixes: 9bb5d40cd93c ("perf: Fix mmap() accounting hole")
> Reported-by: Yang Jihong <yangjihong1@huawei.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Yang Jihong <yangjihong1@huawei.com>
> Link: https://lkml.kernel.org/r/YsQ3jm2GR38SW7uD@worktop.programming.kicks-ass.net
> [YJH: backport to 4.19: adjusted context]
> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  kernel/events/core.c | 45 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)

Sasha already queued this up, thanks.

greg k-h