linux-kernel - Re: [PATCH 00/10] perf/uprobe: Optimize uprobes

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240730131058.GN33588@noisy.programming.kicks-ass.net>
Date: Tue, 30 Jul 2024 15:10:58 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Matthew Wilcox <willy@...radead.org>
Cc: Suren Baghdasaryan <surenb@...gle.com>,
	Andrii Nakryiko <andrii.nakryiko@...il.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Masami Hiramatsu <mhiramat@...nel.org>, mingo@...nel.org,
	andrii@...nel.org, linux-kernel@...r.kernel.org,
	rostedt@...dmis.org, oleg@...hat.com, jolsa@...nel.org,
	clm@...a.com, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 00/10] perf/uprobe: Optimize uprobes

On Sat, Jul 27, 2024 at 04:45:53AM +0100, Matthew Wilcox wrote:

> Hum.  What if we added SLAB_TYPESAFE_BY_RCU to files_cachep?  That way
> we could do:
> 
> 	inode = NULL;
> 	rcu_read_lock();
> 	vma = find_vma(mm, address);
> 	if (!vma)
> 		goto unlock;
> 	file = READ_ONCE(vma->vm_file);
> 	if (!file)
> 		goto unlock;
> 	inode = file->f_inode;
> 	if (file != READ_ONCE(vma->vm_file))
> 		inode = NULL;

remove_vma() does not clear vm_file, nor do I think we ever re-assign
this field after it is set on creation.

That is, I'm struggling to see what this would do. AFAICT this can still
happen:

	rcu_read_lock();
	vma = find_vma();
					remove_vma()
					  fput(vma->vm_file);
								dup_fd)
								  newf = kmem_cache_alloc(...)
								  newf->f_inode = blah

	file = READ_ONCE(vma->vm_file);
	inode = file->f_inode; // blah
	if (file != READ_ONCE(vma->vm_file)) // still match


> unlock:
> 	rcu_read_unlock();
> 
> 	if (inode)
> 		return inode;
> 	mmap_read_lock();
> 	vma = find_vma(mm, address);
> 	...
> 
> I think this would be safe because 'vma' will not be reused while we
> hold the read lock, and while 'file' might be reused, whatever f_inode
> points to won't be used if vm_file is no longer what it once was.


Also, we need vaddr_to_offset() which needs additional serialization
against vma_lock.