Re: [PATCH] mm/kmemleak: Fix sleeping function called from invalid context in kmemleak_seq_show

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 20 Nov 2024 14:53:13 +0000
Catalin Marinas <catalin.marinas@arm.com> 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.

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.

-- Steve