On Mon, Jul 04, 2022 at 08:00:06PM +0800, Yang Jihong wrote:Thanks for reviewing this patch. The perf_mmap_close, perf_event_set_output, and perf_mmap involve complex data race and lock relationships. Therefore, this simple fix is proposed.
Data race exists between perf_event_set_output and perf_mmap_close.
The scenario is as follows:
CPU1 CPU2
perf_mmap_close(event2)
if (atomic_dec_and_test(&event2->rb->mmap_count) // mmap_count 1 -> 0
detach_rest = true;
ioctl(event1, PERF_EVENT_IOC_SET_OUTPUT, event2)
perf_event_set_output(event1, event2)
if (!detach_rest)
goto out_put;
list_for_each_entry_rcu(event, &event2->rb->event_list, rb_entry)
ring_buffer_attach(event, NULL)
// because event1 has not been added to event2->rb->event_list,
// event1->rb is not set to NULL in these loops
ring_buffer_attach(event1, event2->rb)
list_add_rcu(&event1->rb_entry, &event2->rb->event_list)
The above data race causes a problem, that is, event1->rb is not NULL, but event1->rb->mmap_count is 0.
If the perf_mmap interface is invoked for the fd of event1, the kernel keeps in the perf_mmap infinite loop:
again:
mutex_lock(&event->mmap_mutex);
if (event->rb) {
<SNIP>
if (!atomic_inc_not_zero(&event->rb->mmap_count)) {
/*
* Raced against perf_mmap_close() through
* perf_event_set_output(). Try again, hope for better
* luck.
*/
mutex_unlock(&event->mmap_mutex);
goto again;
}
<SNIP>
Too tired, must look again tomorrow, little feeback below.
In this patch, !atomic_read(&rb->mmap_count) is checked before the perf_event_set_output function invokes ring_buffer_attach(event, rb). Therefore, !atomic_read(&rb->mmap_count) does not need to be checked in the ring_buffer_attach function.
kernel/events/core.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 80782cddb1da..c67c070f7b39 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5900,6 +5900,7 @@ static void ring_buffer_attach(struct perf_event *event,
struct perf_buffer *rb)
{
struct perf_buffer *old_rb = NULL;
+ struct perf_buffer *new_rb = rb;
unsigned long flags;
WARN_ON_ONCE(event->parent);
@@ -5928,6 +5929,20 @@ static void ring_buffer_attach(struct perf_event *event,
spin_lock_irqsave(&rb->event_lock, flags);
list_add_rcu(&event->rb_entry, &rb->event_list);
+
+ /*
+ * When perf_mmap_close traverses rb->event_list during
+ * detach all other events, new event may not be added to
+ * rb->event_list, let's check again, if rb->mmap_count is 0,
+ * it indicates that perf_mmap_close is executed.
+ * Manually delete event from rb->event_list and
+ * set event->rb to null.
+ */
+ if (!atomic_read(&rb->mmap_count)) {
+ list_del_rcu(&event->rb_entry);
+ new_rb = NULL;
+ }
+
spin_unlock_irqrestore(&rb->event_lock, flags);
}
@@ -5944,7 +5959,7 @@ static void ring_buffer_attach(struct perf_event *event,
if (has_aux(event))
perf_event_stop(event, 0);
- rcu_assign_pointer(event->rb, rb);
+ rcu_assign_pointer(event->rb, new_rb);
if (old_rb) {
ring_buffer_put(old_rb);
I'm confused by the above hunks; the below will avoid calling
ring_buffer_attach() when !rb->mmap_count, so how can the above ever
execute?
Yes, ring_buffer_put(rb) needs to be added before goto unlock.
@@ -11883,6 +11898,13 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event)
goto unlock;
}
+ /*
+ * If rb->mmap_count is 0, perf_mmap_close is being executed,
+ * the ring buffer is about to be unmapped and cannot be attached.
+ */
+ if (rb && !atomic_read(&rb->mmap_count))
+ goto unlock;
+
ring_buffer_attach(event, rb);
ret = 0;
This is wrong I think, it'll leak ring_buffer_get().
.