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: <20250510094149.7e91736d@gandalf.local.home>
Date: Sat, 10 May 2025 09:41:49 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 bpf@...r.kernel.org, x86@...nel.org, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Josh Poimboeuf <jpoimboe@...nel.org>, Peter Zijlstra
 <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, Jiri Olsa
 <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>
Subject: Re: [PATCH v8 12/18] unwind deferred: Use SRCU
 unwind_deferred_task_work()

On Fri, 9 May 2025 14:49:37 -0700
Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:

> > @@ -133,13 +135,15 @@ static void unwind_deferred_task_work(struct callback_head *head)
> >
> >         timestamp = info->timestamp;
> >
> > -       guard(mutex)(&callback_mutex);
> > -       list_for_each_entry(work, &callbacks, list) {
> > +       idx = srcu_read_lock(&unwind_srcu);  
> 
> nit: you could have used guard(srcu)(&unwind_srcu) ?

Then it would be a scope_guard() as it is only needed for the list. I
prefer using guard() when it is most of the function that is being
protected. Here it's just the list and nothing else.

One issue I have with guard() is that it tends to "leak". That is, if you
use it to protect only one thing and then add more after what you are
protecting, then the guard ends up protecting more than it needs to.

If anything, I would make the loop into its own function with the guard()
then it is more obvious. But for now, I think it's fine as is, unless
others prefer the switch.

-- Steve

> 
> > +       list_for_each_entry_srcu(work, &callbacks, list,
> > +                                srcu_read_lock_held(&unwind_srcu)) {
> >                 if (task->unwind_mask & (1UL << work->bit)) {
> >                         work->func(work, &trace, timestamp);
> >                         clear_bit(work->bit, &current->unwind_mask);
> >                 }
> >         }
> > +       srcu_read_unlock(&unwind_srcu, idx);
> >  }
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ