On Tue, Jul 30, 2024 at 6:11 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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.
