On Wed, Nov 20, 2024 at 10:26:02AM -0500, Steven Rostedt wrote:
> On Wed, 20 Nov 2024 14:53:13 +0000
> Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
> > > -static void print_unreferenced(struct seq_file *seq,
> > > +static depot_stack_handle_t print_unreferenced(struct seq_file *seq,
> > > struct kmemleak_object *object)
> > > {
> > > - int i;
> > > - unsigned long *entries;
> > > - unsigned int nr_entries;
> > > -
> > > - nr_entries = stack_depot_fetch(object->trace_handle, &entries);
> > > warn_or_seq_printf(seq, "unreferenced object 0x%08lx (size %zu):\n",
> > > object->pointer, object->size);
> > > warn_or_seq_printf(seq, " comm \"%s\", pid %d, jiffies %lu\n",
> > > @@ -371,6 +366,23 @@ static void print_unreferenced(struct seq_file *seq,
> > > hex_dump_object(seq, object);
> > > warn_or_seq_printf(seq, " backtrace (crc %x):\n", object->checksum);
> > >
> > > + return object->trace_handle;
> > > +}
> >
> > What I don't fully understand - is this a problem with any seq_printf()
> > or just the backtrace pointers from the stack depot that trigger this
> > issue? I guess it's something to do with restricted pointers but I'm not
> > familiar with the PREEMPT_RT concepts. It would be good to explain,
> > ideally both in the commit log and a comment in the code, why we only
> > need to do this for the stack dump.
>
> In PREEMPT_RT, to achieve the ability to preempt in more context,
> spin_lock() is converted to a special sleeping mutex. But there's some
> places where it can not be converted, and in those cases we use
> raw_spin_lock(). kmemleak has been converted to use raw_spin_lock() which
> means anything that gets called under that lock can not take a normal
> spin_lock().
>
> What happened here is that the kmemleak raw spinlock is held and
> seq_printf() is called. Normally, this is not an issue, but the behavior of
> seq_printf() is dependent on what values is being printed.
>
> The "%pK" dereferences a pointer and there's some SELinux hooks attached to
> that code. The problem is that the SELinux hooks take spinlocks. This would
> not have been an issue if it wasn't for that "%pK" in the format.
Thanks Steven. That's very useful.
> Maybe SELinux locks should be converted to raw? I don't know how long that
> lock is held. There are some loops though :-/
>
> avc_insert():
>
> spin_lock_irqsave(lock, flag);
> hlist_for_each_entry(pos, head, list) {
> if (pos->ae.ssid == ssid &&
> pos->ae.tsid == tsid &&
> pos->ae.tclass == tclass) {
> avc_node_replace(node, pos);
> goto found;
> }
> }
> hlist_add_head_rcu(&node->list, head);
> found:
> spin_unlock_irqrestore(lock, flag);
>
> Perhaps that could be converted to simple RCU?
>
> As I'm sure there's other places that call vsprintf() under a raw_spin_lock
> or non-preemptable context, perhaps this should be the fix we do.
My preference would also be to convert SELinux rather than avoiding the
issue in kmemleak (and other similar places).
--
Catalin
