From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0D81CECE579 for ; Mon, 9 Sep 2024 13:13:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3107B6B016F; Mon, 9 Sep 2024 09:13:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2C0556B0170; Mon, 9 Sep 2024 09:13:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 188756B0171; Mon, 9 Sep 2024 09:13:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id EFB7A6B016F for ; Mon, 9 Sep 2024 09:13:39 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 70161A032C for ; Mon, 9 Sep 2024 13:13:39 +0000 (UTC) X-FDA: 82545241758.17.62B0006 Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) by imf08.hostedemail.com (Postfix) with ESMTP id 52652160016 for ; Mon, 9 Sep 2024 13:13:37 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=KNKTcBUa; spf=pass (imf08.hostedemail.com: domain of jannh@google.com designates 209.85.167.54 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725887590; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=e0Qpk+x67S20AfnP0HkE2Lz8EGrKLhCwgTYyWP4OIYE=; b=cEWU5hry/wzaWkkuCc8AU5PEV7kUTGnwD0rP8kicj0zrzp9NDVGcqpLsFOeS2wXVnDZ1F6 zlWX75Hef/93e+iMma1Gvbn8xu9FzWsZg2KbHs3ekIdinjIT2GXEbm+cToToQq/RuNNNw8 E4TBPcV4f7CY5U3W23tYCjRp8nQxjb8= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=KNKTcBUa; spf=pass (imf08.hostedemail.com: domain of jannh@google.com designates 209.85.167.54 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725887590; a=rsa-sha256; cv=none; b=OA5EKH/IXkP43J84ktFnA5k3i02i9iFplpcd0/AhXbM80A7d4y5R/tPlx2ZxzUc2o4cb/5 FCTL0nzkE6VwFby/tJD03FLab8vRomemmcaZB2YSRiTu8KpcK//E0QvzovjwzlDymbzSuQ TEtpZ43pD3LKRD6jRhDBXxJ7ZJ0gxNU= Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-5356a2460ceso9070e87.0 for ; Mon, 09 Sep 2024 06:13:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1725887615; x=1726492415; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=e0Qpk+x67S20AfnP0HkE2Lz8EGrKLhCwgTYyWP4OIYE=; b=KNKTcBUaci15VTIXxhLDVNyv16+coujoCDa49ziYDx/wjCDZfQ/5nQy/DbjjbCm2DE AS/Hj4TLIwVx1XDOvR9/mD222bCD2HRLxJgB5lnFaHhMlTdu1MXEzz/vR1ZZBtDNaU1x bURPq/tqodV7bMd8HhXKBZ6uBFyW008IpEzoDXpgGIlWQy/bE8ODWtn3jOr2RWriaFR1 MWHuamiZBBtZltGFV2l9eGh0Q9Tp+zoBF07MTJupVdsNdqsmZ+h1RJgE8Ac5+qvIASTf 4+7C5IkC2FEmWTKAmV5XPYxCTu+5ZT3Y9QM+FlXv52tFolWhNgdzzLaGiTi4EIZRF8pu qwHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725887615; x=1726492415; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=e0Qpk+x67S20AfnP0HkE2Lz8EGrKLhCwgTYyWP4OIYE=; b=Fwp9paNwn8oyKu8OZWaD9ZjzERxUZhV55q9hyWob1l761zKAczqnaGyV4a1RBL/8rF ARJmi2zNcgSz9vRKHrU/kGPI3suYZaBzwHr58xaexg2Rg4m0pGPo7iY84+mYyzVbyeme Bi03p26fokhlrAy1/4igCvPUCdzArOXPYh5s/L4KI961kIqJXvKIoCxl6W85l+z5Nz+a 87iHG2V+J8gc+mc6CkL2HXXpyDwEkF+NjnoGBc51i6hFbBgNuGPE3zEFEUXFdIRgS96Q 62Ki4mNGWXfsF4p4zLsx+I07FuFs+dm+8AKG4kyqt3Og1WRjN+inLaRhBoKrksKGm8BP wBog== X-Forwarded-Encrypted: i=1; AJvYcCUJnBKoHuRZv9EZBLZNIYsaei45+szfp5UqPuSSKory57j19rTZxnIqL28zu11ilDPBTHmK7j5UFA==@kvack.org X-Gm-Message-State: AOJu0Yzjt5BBXVMg3KAhOngQEChrA3Ao/cLA8vWtwLO8EiR6aMwP4jWs 4UBxZqFwnVJnuwi4Yqey4PXljPNPxjF2smq5Q4AlJd0WT3kU0jJlzeKb0mRpqqAuU9bg2vclFhX dLqnmIQwQHZP2eJ0eY6IFVrzBZ2sesFHDADpg X-Google-Smtp-Source: AGHT+IGbHTgjUP9n9e0Je4tUVw4yQi27pYfPz7pOzFc9OB1SgZi153AKL0qkRB1p9AOL0pdG0v4kJX60dSJe2NNaGII= X-Received: by 2002:a05:6512:b05:b0:52e:8a42:f152 with SMTP id 2adb3069b0e04-5365eb6fa31mr222153e87.5.1725887614987; Mon, 09 Sep 2024 06:13:34 -0700 (PDT) MIME-Version: 1.0 References: <20240906051205.530219-1-andrii@kernel.org> <20240906051205.530219-3-andrii@kernel.org> In-Reply-To: <20240906051205.530219-3-andrii@kernel.org> From: Jann Horn Date: Mon, 9 Sep 2024 15:12:57 +0200 Message-ID: Subject: Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution To: Andrii Nakryiko , brauner@kernel.org Cc: linux-trace-kernel@vger.kernel.org, peterz@infradead.org, oleg@redhat.com, rostedt@goodmis.org, mhiramat@kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org, jolsa@kernel.org, paulmck@kernel.org, willy@infradead.org, surenb@google.com, akpm@linux-foundation.org, linux-mm@kvack.org, mjguzik@gmail.com, Miklos Szeredi , Amir Goldstein , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: hktgh8gzsgmx7tr5gzjtymahzc73gdsc X-Rspamd-Queue-Id: 52652160016 X-Rspamd-Server: rspam11 X-HE-Tag: 1725887617-280494 X-HE-Meta: U2FsdGVkX1/5U58T/EVhfLJNCLuv7hA4N2qOtet8wR1ae4L/EPmT5URHq/YVdOJ4QlJab26CHoLipMX2CYQpaHxUdVfOwOxc+C8oRK8SOAH2OgjGKQqm+23rAbqKni+9AYvQDYGQ86Ri8lDo/XZuhyUW4kOxkBeGzNcuWfwinZ7hjGFPVyzABF3UO12WwERADzXq4rEH1Whot8+D7y+Xv7OoQukQ82PLPa4SPHJanX0FInKtOMuLJ6ijlaCAEaioWUxTIn0gk5xbO8zdlVXOG725qqsX6EnHtayMgFnzbs+XO5nXC+Sp8WSenPw0HZEGSRsGZ74KbiHlralwj3vf0qTlg706QGvqEumlhN6wYxRPDTYvBJ7MENJ2ZwR4+tZ0IfgcmVC/o3+G3W9+x4sLFT2Fj7PAQZhN0Z7XjYN/ApptE3DZMoZDa33X4Irvt5Bp25rFgEIANaHTDaIDNQl4q5u8ESYzCWy/FSgb/GUTBjiN7xwLybsVtLcXoNiLOCHJtDrmbBmKviwHGH2+MalTQOHgZ8mrHjMWje3bZI+kf6zyIRjqMnUhXBNCJcowxIplFnZQOvPQ6ieCeHTtI+0PR96o/fo8UZ+MHtqmzc0xTO9zTkDrpjARO1NidZWVuM6F5SsE2c+bcKAE/0IHMFixfrlWDnxmon1c/9ErxvC8ZV473eDJQvdZPlFNv1blTc6Oav0ctDye9K30AiRkguHSs66FrSxmacHXsrK1He1hKpl/bzEM+Z4vDv3U1GzwAEDd2zQnpToiae7s+BIbriLYs4c7td8aJFx4bfIOAFfIyKgVK+IRdBM/5xzoQkZJeKfe+0xLUCqSNMSpHdVQW5pVwaJK1FPRfmgs4aoHDF9mjnZWSFlyLic4a0RRZWdXukMPM0zc3HOFs8Js1VgHTgfCeqtEfdVCb1fa0yTEx3Knmds8V6YP0lmx0eMUrLmiKFP9NA3FIGLIwfgYP4Wu/zA SEBzk1Zs 20DpIQb9iU0AlBl//UxG0XOFTwqOs3LL/QUOcQKgfhIFb7dKGuJYaTJuR8iTI4dex9u/fXtKBwHS0Po7tkd60pAdduJNyusCmqSS45cebn1AveizeJDrrX7jAOnUbpJhZItdWfE3g1nOpWXvPNr9Y6Xz8AdEcyScvVqjn+h19kWZcTKyzQObpyqDguYksio28GZMbRROUQ8Ta3ZwRg8q4y2h6/H8XJNyxWyao X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Sep 6, 2024 at 7:12=E2=80=AFAM Andrii Nakryiko = wrote: > Given filp_cachep is already marked SLAB_TYPESAFE_BY_RCU, we can safely > access vma->vm_file->f_inode field locklessly under just rcu_read_lock() No, not every file is SLAB_TYPESAFE_BY_RCU - see for example ovl_mmap(), which uses backing_file_mmap(), which does vma_set_file(vma, file) where "file" comes from ovl_mmap()'s "realfile", which comes from file->private_data, which is set in ovl_open() to the return value of ovl_open_realfile(), which comes from backing_file_open(), which allocates a file with alloc_empty_backing_file(), which uses a normal kzalloc() without any RCU stuff, with this comment: * This is only for kernel internal use, and the allocate file must not be * installed into file tables or such. And when a backing_file is freed, you can see on the path __fput() -> file_free() that files with FMODE_BACKING are directly freed with kfree(), no RCU delay= . So the RCU-ness of "struct file" is an implementation detail of the VFS, and you can't rely on it for ->vm_file unless you get the VFS to change how backing file lifetimes work, which might slow down some other workload, or you find a way to figure out whether you're dealing with a backing file without actually accessing the file. > +static struct uprobe *find_active_uprobe_speculative(unsigned long bp_va= ddr) > +{ > + const vm_flags_t flags =3D VM_HUGETLB | VM_MAYEXEC | VM_MAYSHARE; > + struct mm_struct *mm =3D current->mm; > + struct uprobe *uprobe; > + struct vm_area_struct *vma; > + struct file *vm_file; > + struct inode *vm_inode; > + unsigned long vm_pgoff, vm_start; > + int seq; > + loff_t offset; > + > + if (!mmap_lock_speculation_start(mm, &seq)) > + return NULL; > + > + rcu_read_lock(); > + > + vma =3D vma_lookup(mm, bp_vaddr); > + if (!vma) > + goto bail; > + > + vm_file =3D data_race(vma->vm_file); A plain "data_race()" says "I'm fine with this load tearing", but you're relying on this load not tearing (since you access the vm_file pointer below). You're also relying on the "struct file" that vma->vm_file points to being populated at this point, which means you need CONSUME semantics here, which READ_ONCE() will give you, and something like RELEASE semantics on any pairing store that populates vma->vm_file, which means they'd all have to become something like smp_store_release()). You might want to instead add another recheck of the sequence count (which would involve at least a read memory barrier after the preceding patch is fixed) after loading the ->vm_file pointer to ensure that no one was concurrently changing the ->vm_file pointer before you do memory accesses through it. > + if (!vm_file || (vma->vm_flags & flags) !=3D VM_MAYEXEC) > + goto bail; missing data_race() annotation on the vma->vm_flags access > + vm_inode =3D data_race(vm_file->f_inode); As noted above, this doesn't work because you can't rely on having RCU lifetime for the file. One *very* ugly hack you could do, if you think this code is so performance-sensitive that you're willing to do fairly atrocious things here, would be to do a "yes I am intentionally doing a UAF read and I know the address might not even be mapped at this point, it's fine, trust me" pattern, where you use copy_from_kernel_nofault(), kind of like in prepend_copy() in fs/d_path.c, and then immediately recheck the sequence count before doing *anything* with this vm_inode pointer you just loaded. > + vm_pgoff =3D data_race(vma->vm_pgoff); > + vm_start =3D data_race(vma->vm_start); > + > + offset =3D (loff_t)(vm_pgoff << PAGE_SHIFT) + (bp_vaddr - vm_star= t); > + uprobe =3D find_uprobe_rcu(vm_inode, offset); > + if (!uprobe) > + goto bail; > + > + /* now double check that nothing about MM changed */ > + if (!mmap_lock_speculation_end(mm, seq)) > + goto bail; > + > + rcu_read_unlock(); > + > + /* happy case, we speculated successfully */ > + return uprobe; > +bail: > + rcu_read_unlock(); > + return NULL; > +}