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 4D1A1EEB591 for ; Thu, 12 Sep 2024 11:17:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C88156B00B2; Thu, 12 Sep 2024 07:17:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C0B796B00B4; Thu, 12 Sep 2024 07:17:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A80216B00B5; Thu, 12 Sep 2024 07:17:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 893AC6B00B2 for ; Thu, 12 Sep 2024 07:17:21 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 4C572141BAC for ; Thu, 12 Sep 2024 11:17:21 +0000 (UTC) X-FDA: 82555835082.28.277CB4E Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf03.hostedemail.com (Postfix) with ESMTP id 8F3A52000E for ; Thu, 12 Sep 2024 11:17:19 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=nlvOBuDs; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf03.hostedemail.com: domain of brauner@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=brauner@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1726139723; 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=SvSjOcKpyR5JdwzORZy5kvPcMe3LfmIiVxD+Km69BQ8=; b=TszjIbyPrcLcGeUZQ+Hnf4UHY0Rrwpz+O81m3myd2x6aR6yKwYPD0hiCoTHyeDoPMdoVIQ sZKS4o/O8uIWmYo1RP+tYjMvr/CK0XeYY7RN+DVKd7s4pMgqNNa/J6/RDiX1EBQVLjGjpL /VDXxIMCkr5GxrQ461/s9w0tEazYvd8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726139723; a=rsa-sha256; cv=none; b=srnj+wWVXpnuhOJTkBjP6q4MpfOhon8GGYqIHCimRNaDhhdWN+k3LO90MdV2yuFLMu22l1 r8TbhJ3eXSKniGdO9Q9cmDuJc454gdR7cLJV/KG/tQw+quYde+XpCoEeUypzDBQz06rdvC 1r5xXCj8pk3El5cYkCb5UHNvZ+85q8s= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=nlvOBuDs; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf03.hostedemail.com: domain of brauner@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=brauner@kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 3B8525C5AB3; Thu, 12 Sep 2024 11:17:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BF1F8C4CEC5; Thu, 12 Sep 2024 11:17:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1726139838; bh=ASKEujshUwUAQKq7+qtYTggR4rpjzptS8JkTy/cZSEM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=nlvOBuDsUhk42rm1aN38HY8iYcNeevzjnrJW42FM3qKLjNwUjR3iCgiXX+eR6O5AG VA9JPJIsRDoJ0Sxt7rZrc11tUxJtCcRP67ykNKltP1HEb5Cl8iqkCzHgj2cvA/Wlcx NsqV0R9Vrch1GImU+BPyTy2m3Mh7+xTBzszts4NY8k9cgB1VnH80wegyzZHsvSuQkF WKDmz8DLYcveuCB87d1XfE2nmY54fNoHrSZBaC9A681AJYUjtLlwqmq6ypO46/gz3R xvtTZ9VsIVBCRPILOxAeucTt3FRX1Ja8w9YgpNPGJwiwcUBFFkFT1oZNGO217ff79G T34w5rvn3G4VA== Date: Thu, 12 Sep 2024 13:17:10 +0200 From: Christian Brauner To: Andrii Nakryiko Cc: Suren Baghdasaryan , Jann Horn , Liam Howlett , Andrii Nakryiko , 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, akpm@linux-foundation.org, linux-mm@kvack.org, mjguzik@gmail.com, Miklos Szeredi , Amir Goldstein , linux-fsdevel Subject: Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution Message-ID: <20240912-urenkel-umorientieren-c27ce893af09@brauner> References: <20240906051205.530219-1-andrii@kernel.org> <20240906051205.530219-3-andrii@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 8F3A52000E X-Stat-Signature: 54pwrpsdnzmjk8x9z53xqm57cpgxqajb X-Rspam-User: X-HE-Tag: 1726139839-173001 X-HE-Meta: U2FsdGVkX1/Fg/Uu887ZyQvLLko4teHn6eOjGmgv0jxR/SSRNd0bCNgCtZNU2K9kujDVI5RSplNUHQqEqjlK1kfptERD3eY2uCJZum/4AF58DFE+7YSN+WkB6BXApgqHn6xMjbVwzLtjdAqwlY7aPMcWMYi6lzi3If+0q4uwOfSVAoR+Isc70NtMlcCXGtfZOpT8UnnhuMuxhwmYnr31gKtcBYWNW99ceevBBEWiquMCR1nzJEr6E9XhNcmaiHgWiJP0bOtQrioGjZT1r0UaORPKkHUrEuoWtwNBDFahzHa22rd0e1WkUeMLgYEOSa/bG5KxnPotE4v1/y96J8e60zINC4Vsv7jJdCVmzZZbkHZQdt5uPkeUkQuR237M7ceuWZHaTYnpgUC9zGixJchBAZkKqE3A+URECwew2JPDplNmKqbIVWyB1Bkj+KdDWes1GJiF+IPPbOc4kl/C+8YA0IcJHLgnvpRgl28OYptSmGw7vW7gjQJwUh+1KoDBNbM+xyh+yFX2Nk4Iv+GzchFsgDfruaHH/Ct0pVxj5pTdR8egdaqDklw2RAa6K+z/Q6aSMaTtfgGpdJm3KpEEWkbJZvQ8IJxLrZ8nLgMYaUGy59Ex7Po/s1odW6zxTq4J41+XaCks9PJCKOng6AyrfG5MfUaXNI5w366oWq2s39LnQF0jVu0yvtR+XeZ7Zc6Qal3RNFaQ/v0fYa8LQHWq0dXtVdrPhtukV+Ha9fxJAxJYqizVxmH9Enm17hmEl5PTS/zZeliaD+RlNma5U+u6YcOxEXTdtQ2qm59nhMf5OYp4urGbqTXvBB87HJ80iVLP60Yi65fxKnf6vwwWPw0nmK80Aj1atOpXBuVqtNcE7oxjAA3xylbzVDogpbqpU+b3yrOKNmsoy0HhYrlC4dU37pQCKhx9abelz16leVWCwnWFoKrf1vOfMOWfqjsmaTAdrJJP14ozeS6tzAzYAqzSLbf qykyfacv f/xlheJ5vsiucTplGxZYNvX639w+4tjfqwQVWIdaeS3gpKqb+eePhyd9AbGLbgbEBds+5Uo7TH6wLFkSOQnQHXJtvEeqQlD/FepBqkmBOZRMh49mnpjH/SFVTTvSNqnSkT2NCdGaDzNKXXMmry1XWRGq0cwB2thlFID7PkN8MTVfnsYdRaF5zPlnzH3Eg/nP3ZcVzjWYpcshv/dTM/yM1pPx4WsBgmgCUKOtYHvrqhHLyclxJW2Nq9Ja/QQF66J7PEllar7K7J5WaflICMq2q1dPZAyzd8Rfa8oAIi2cFduZBel96B3545Z9QKdg/19whAkcj9Po38vbKPGHBLsVHysAQLo1kgFOHTtSgIN9dMcoRXShvU3L8e6/asMZouI2kW02O3hhyrE9wSWMo8KTOFP4hPrqK2do+8/fESkjZXQ6K/QJdbvBzhJjoisDTYPjfInU6RLzb/tCCI4pooFDwwCiK8tbqgtIUYCHq 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 Tue, Sep 10, 2024 at 01:58:10PM GMT, Andrii Nakryiko wrote: > On Tue, Sep 10, 2024 at 9:32 AM Suren Baghdasaryan wrote: > > > > On Mon, Sep 9, 2024 at 2:29 PM Andrii Nakryiko > > wrote: > > > > > > On Mon, Sep 9, 2024 at 6:13 AM Jann Horn wrote: > > > > > > > > On Fri, Sep 6, 2024 at 7:12 AM 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. > > > > > > Good catch on FMODE_BACKING, I didn't realize there is this exception, thanks! > > > > > > I think the way forward would be to detect that the backing file is in > > > FMODE_BACKING and fall back to mmap_lock-protected code path. > > > > > > I guess I have the question to Liam and Suren, do you think it would > > > be ok to add another bool after `bool detached` in struct > > > vm_area_struct (guarded by CONFIG_PER_VMA_LOCK), or should we try to > > > add an extra bit into vm_flags_t? The latter would work without > > > CONFIG_PER_VMA_LOCK, but I don't know what's acceptable with mm folks. > > > > > > This flag can be set in vma_set_file() when swapping backing file and > > > wherever else vma->vm_file might be set/updated (I need to audit the > > > code). > > > > I understand that this would work but I'm not very eager to leak > > vm_file attributes like FMODE_BACKING into vm_area_struct. > > Instead maybe that exception can be avoided? Treating all vm_files > > I agree, that would be best, of course. It seems like [1] was an > optimization to avoid kfree_rcu() calls, not sure how big of a deal it > is to undo that, given we do have a use case that calls for it now. > Let's see what Christian thinks. Do you just mean? diff --git a/fs/file_table.c b/fs/file_table.c index 7ce4d5dac080..03e58b28e539 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -68,7 +68,7 @@ static inline void file_free(struct file *f) put_cred(f->f_cred); if (unlikely(f->f_mode & FMODE_BACKING)) { path_put(backing_file_user_path(f)); - kfree(backing_file(f)); + kfree_rcu(backing_file(f)); } else { kmem_cache_free(filp_cachep, f); } Then the only thing you can do with FMODE_BACKING is to skip it. I think that should be fine since backing files right now are only used by overlayfs and I don't think the kfree_rcu() will be a performance issue. > > > equally as RCU-safe would be a much simpler solution. I see that this > > exception was introduced in [1] and I don't know if this was done for > > performance reasons or something else. Christian, CCing you here to > > please clarify. > > > > [1] https://lore.kernel.org/all/20231005-sakralbau-wappnen-f5c31755ed70@brauner/ > > > > > > > > > > > > > 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_vaddr) > > > > > +{ > > > > > + const vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC | VM_MAYSHARE; > > > > > + struct mm_struct *mm = 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 = vma_lookup(mm, bp_vaddr); > > > > > + if (!vma) > > > > > + goto bail; > > > > > + > > > > > + vm_file = 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()). > > > > > > vma->vm_file should be set in VMA before it is installed and is never > > > modified afterwards, isn't that the case? So maybe no extra barrier > > > are needed and READ_ONCE() would be enough. > > > > > > > > > > > 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) != VM_MAYEXEC) > > > > > + goto bail; > > > > > > > > missing data_race() annotation on the vma->vm_flags access > > > > > > ack > > > > > > > > > > > > + vm_inode = 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. > > > > > > > > > > > > > > yeah, let's leave it as a very unfortunate plan B and try to solve it > > > a bit cleaner. > > > > > > > > > > > > > > > + vm_pgoff = data_race(vma->vm_pgoff); > > > > > + vm_start = data_race(vma->vm_start); > > > > > + > > > > > + offset = (loff_t)(vm_pgoff << PAGE_SHIFT) + (bp_vaddr - vm_start); > > > > > + uprobe = 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; > > > > > +}