On 10/01, Andrii Nakryiko wrote:
>
> +static struct uprobe *find_active_uprobe_speculative(unsigned long bp_vaddr)
> +{
> + struct mm_struct *mm = current->mm;
> + struct uprobe *uprobe = NULL;
> + struct vm_area_struct *vma;
> + struct file *vm_file;
> + loff_t offset;
> + long seq;
> +
> + guard(rcu)();
> +
> + if (!mmap_lock_speculation_start(mm, &seq))
> + return NULL;
> +
> + vma = vma_lookup(mm, bp_vaddr);
> + if (!vma)
> + return NULL;
> +
> + /* vm_file memory can be reused for another instance of struct file,
> + * but can't be freed from under us, so it's safe to read fields from
> + * it, even if the values are some garbage values; ultimately
> + * find_uprobe_rcu() + mmap_lock_speculation_end() check will ensure
> + * that whatever we speculatively found is correct
> + */
> + vm_file = READ_ONCE(vma->vm_file);
> + if (!vm_file)
> + return NULL;
> +
> + offset = (loff_t)(vma->vm_pgoff << PAGE_SHIFT) + (bp_vaddr -
> vma->vm_start);
LGTM. But perhaps vma->vm_pgoff and vma->vm_start need READ_ONCE() as well,
if nothing else to shut up KCSAN if this code races with, say, __split_vma() ?
> + uprobe = find_uprobe_rcu(vm_file->f_inode, offset);
OK, I guess vm_file->f_inode is fine without READ_ONCE...
Oleg.
