On 5/7/2024 4:41 PM, Peter Zijlstra wrote:
> On Mon, May 06, 2024 at 05:29:31AM +0000, Mingwei Zhang wrote:
>
>> +int perf_get_mediated_pmu(void)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&perf_mediated_pmu_mutex);
>> + if (refcount_inc_not_zero(&nr_mediated_pmu_vms))
>> + goto end;
>> +
>> + if (atomic_read(&nr_include_guest_events)) {
>> + ret = -EBUSY;
>> + goto end;
>> + }
>> + refcount_set(&nr_mediated_pmu_vms, 1);
>> +end:
>> + mutex_unlock(&perf_mediated_pmu_mutex);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(perf_get_mediated_pmu);
>> +
>> +void perf_put_mediated_pmu(void)
>> +{
>> + if (!refcount_dec_not_one(&nr_mediated_pmu_vms))
>> + refcount_set(&nr_mediated_pmu_vms, 0);
>
> I'm sorry, but this made the WTF'o'meter go 'ding'.
>
> Isn't that simply refcount_dec() ?
when nr_mediated_pmu_vms is 1, refcount_dec(&nr_mediated_pmu_vms) has an
error and call trace: refcount_t: decrement hit 0; leaking memory.
Similar when nr_mediated_pmu_vms is 0, refcount_inc(&nr_mediated_pmu_vms)
has an error and call trace also: refcount_t: addition on 0; use_after_free.
it seems refcount_set() should be used to set 1 or 0 to refcount_t.
>
>> +}
>> +EXPORT_SYMBOL_GPL(perf_put_mediated_pmu);
>> +
>> /*
>> * Holding the top-level event's child_mutex means that any
>> * descendant process that has inherited this event will block
>> @@ -12086,11 +12140,24 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>> if (err)
>> goto err_callchain_buffer;
>>
>> + if (is_include_guest_event(event)) {
>> + mutex_lock(&perf_mediated_pmu_mutex);
>> + if (refcount_read(&nr_mediated_pmu_vms)) {
>> + mutex_unlock(&perf_mediated_pmu_mutex);
>> + err = -EACCES;
>> + goto err_security_alloc;
>> + }
>> + atomic_inc(&nr_include_guest_events);
>> + mutex_unlock(&perf_mediated_pmu_mutex);
>> + }
>
> Wouldn't all that be nicer with a helper function?
yes, it is nicer.
thanks
>
> if (is_include_guest_event() && !perf_get_guest_event())
> goto err_security_alloc;
>
>> +
>> /* symmetric to unaccount_event() in _free_event() */
>> account_event(event);
>>
>> return event;
>>
>> +err_security_alloc:
>> + security_perf_event_free(event);
>> err_callchain_buffer:
>> if (!event->parent) {
>> if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
>> --
>> 2.45.0.rc1.225.g2a3ae87e7f-goog
>>
>
