Re: [PATCH v4 0/9] uprobes: misc cleanups/simplifications

[Adrian Hunter <adrian.hunter@intel.com>]

On 2/08/24 14:02, Adrian Hunter wrote:
> On 2/08/24 12:25, Peter Zijlstra wrote:
>> On Thu, Aug 01, 2024 at 02:13:41PM -0700, Andrii Nakryiko wrote:
>>
>>> Ok, this bisected to:
>>>
>>> 675ad74989c2 ("perf/core: Add aux_pause, aux_resume, aux_start_paused")
>>
>> Adrian, there are at least two obvious bugs there:
>>
>>  - aux_action was key's off of PERF_PMU_CAP_AUX_OUTPUT, which is not
>>    right, that's the capability where events can output to AUX -- aka.
>>    PEBS-to-PT. It should be PERF_PMU_CAP_ITRACE, which is the
>>    PT/CoreSight thing.

Not sure about that.

In perf_event_alloc(), there is:

	if (event->attr.aux_output &&
	    (!(pmu->capabilities & PERF_PMU_CAP_AUX_OUTPUT) ||
	     event->attr.aux_pause || event->attr.aux_resume)) {
		err = -EOPNOTSUPP;
		goto err_pmu;
	}

which is to prevent aux_output with aux_pause or aux_resume.
That is because aux_output (i.e. PEBS-via-PT) has no interrupt
and so does not overflow.  (Instead the PEBS record is written
by hardware to the Intel PT trace)  No overflow => no (software)
aux_pause/aux_resume, so aux_output with aux_pause/aux_resume
does not make sense.

The PMU capability for aux_pause/aux_resume or aux_start_paused
is PERF_PMU_CAP_AUX_PAUSE.  aux_pause/aux_resume are valid for
non-AUX events (member of the group), whereas aux_start_paused
is valid for the AUX event itself (group leader).  For 
aux_pause/aux_resume the group leader's PMU capability is
checked.  For aux_start_paused the event's PMU capability is
checked.

>>
>>  - it sets aux_paused unconditionally, which is scribbling in the giant
>>    union which is overwriting state set by perf_init_event().

That definitely needs fixing, but the fix is just the diff
from my previous reply:

diff --git a/kernel/events/core.c b/kernel/events/core.c
index e4cb6e5a5f40..2072aaa4d449 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12151,7 +12151,8 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 		err = -EOPNOTSUPP;
 		goto err_pmu;
 	}
-	event->hw.aux_paused = event->attr.aux_start_paused;
+	if (event->attr.aux_start_paused)
+		event->hw.aux_paused = 1;
 
 	if (cgroup_fd != -1) {
 		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);

I tested that with:

  # perf probe -x /root/main -a main
  Added new event:
    probe_main:main      (on main in /root/main)

  # perf record -e probe_main:main -- ./main

and it made the problem go away.

>>
>> But I think there's more problems, we need to do the aux_action
>> validation after perf_get_aux_event(), we can't know if having those
>> bits set makes sense before that. This means the perf_event_alloc() site
>> is wrong in the first place.

As above, aux_start_paused is used on the AUX event itself, so the
PMU capability is checked in perf_event_alloc:

	if (event->attr.aux_start_paused &&
	    !(pmu->capabilities & PERF_PMU_CAP_AUX_PAUSE)) {
		err = -EOPNOTSUPP;
		goto err_pmu;
	}

Whereas aux_pause/aux_resume are checked in perf_get_aux_event():

	if ((event->attr.aux_pause || event->attr.aux_resume) &&
	    !(group_leader->pmu->capabilities & PERF_PMU_CAP_AUX_PAUSE))
		return 0;

That all seems OK, so please let me know if there is
something else to change.