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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAJuCfpFUQFfgx0BWdkNTAiOhBpqmd02zarC0y38gyB5OPc0wRA@mail.gmail.com>
Date: Tue, 30 Jul 2024 11:10:33 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Matthew Wilcox <willy@...radead.org>, 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 Tue, Jul 30, 2024 at 6:11 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> 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.

Quite correct and even if we clear vm_file in remove_vma() and/or
reset it on creation I don't think that would be enough. IIUC the
warning about SLAB_TYPESAFE_BY_RCU here:
https://elixir.bootlin.com/linux/v6.10.2/source/include/linux/slab.h#L98
means that the vma object can be reused in the same RCU grace period.

>
> 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
>

Imagine that the vma got freed and reused at this point. Then
vma->vm_file might be pointing to a valid but a completely different
file.

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

I think we should also check that the VMA represents the same area
after we obtained the inode.

>
>
> > 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.