linux-kernel - Re: [bug report] perf: Fix event leak upon exec and file release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zv1P7UzPWvFjOupc@localhost.localdomain>
Date: Wed, 2 Oct 2024 15:51:41 +0200
From: Frederic Weisbecker <frederic@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
	Peter Zijlstra <peterz@...radead.org>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [bug report] perf: Fix event leak upon exec and file release

Le Mon, Sep 30, 2024 at 11:04:39AM +0200, Sebastian Andrzej Siewior a écrit :
> On 2024-09-02 23:47:08 [+0200], Frederic Weisbecker wrote:
> > Ah right.
> > 
> > So one possible fix is to possibly let the task work do the last reference
> > decrement. This would mean that freeing children events can't be always assumed
> > by the parent.
> > 
> > The below (only built tested) would do it?
> 
> This looks nice based on diffstat. I didn't look closed nor did any
> testing so far. Do we like it and wait for Dan here or?

No eventually I think the issues reported by Dan can't happen because:

pl330_free_chan_resources() <- disables preempt
-> pl330_release_channel()
   -> _free_event()
      -> perf_pending_task_sync()

That's another _free_event that is not related to perf.

Two and three:
perf_remove_from_context() <- disables preempt
__perf_event_exit_context() <- disables preempt
-> __perf_remove_from_context()
   -> perf_group_detach()
      -> perf_put_aux_event()
         -> put_event()
            -> _free_event()
               -> perf_pending_task_sync()

Four:
perf_free_event() <- disables preempt
-> perf_group_detach()
   -> perf_put_aux_event()
      -> put_event()
         -> _free_event()
            -> perf_pending_task_sync()


The put_event() calls here and above can't be the last ones. Because
if an event is attached to an aux event, detaching from it means that
aux event itself hasn't even gone through perf_event_remove_from_context().

Similarly an exiting aux event detaching the events from it means those attached
events haven't gone through perf_event_remove_from_context().

So this should be fine (famous last words). There is a might_sleep() call in
irq_work_sync() that should tell us about it.

Thanks.