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 E4ED2ECE589 for ; Mon, 9 Sep 2024 21:29:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 50A696B00CD; Mon, 9 Sep 2024 17:29:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4BAE26B020C; Mon, 9 Sep 2024 17:29:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 35AE68D0001; Mon, 9 Sep 2024 17:29:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 1377D6B00CD for ; Mon, 9 Sep 2024 17:29:20 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 70BC4140418 for ; Mon, 9 Sep 2024 21:29:19 +0000 (UTC) X-FDA: 82546490838.26.8D8E969 Received: from mail-pl1-f169.google.com (mail-pl1-f169.google.com [209.85.214.169]) by imf17.hostedemail.com (Postfix) with ESMTP id 923154000B for ; Mon, 9 Sep 2024 21:29:17 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=VNpu7vrJ; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf17.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.214.169 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725917244; 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=PWEX4r7jK/WQUNLo3UpE/qaecNNCCFHIzAO2V+o4c/g=; b=6/fZwiFmwjH9vhXu3PWf4kCEyPSs50bXTmMfZkGFnr7Bd3VCziaksnrYdjxKIfiE1dkATA XxW4O3rMy2hKM9lneJBrWEMyqAY37OikB0wBm9Hgsr7gBTrZKAwLBXr0bbcebh4j58Y/ro +WhYS3mBM0ZMEx6oHtHQKT1NriTmG0A= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725917244; a=rsa-sha256; cv=none; b=iGA3Ez4mFdb7m/Rhge3RVQaAFQCKEYCO5SQjdtFaBWZPgF+cbMbZRvFGkiAnvNAw3BoZGp NJdp+c2ZB5PkfbP68KUjT/hlEj9rdhQRQwAhkkHh8mfIJ9KaYnu9tbf2HDYM37WxjSWANa v+Jt4vaaHD0lAqKWbk0qrlwm9ebvliU= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=VNpu7vrJ; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf17.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.214.169 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com Received: by mail-pl1-f169.google.com with SMTP id d9443c01a7336-20696938f86so40736165ad.3 for ; Mon, 09 Sep 2024 14:29:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1725917356; x=1726522156; 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=PWEX4r7jK/WQUNLo3UpE/qaecNNCCFHIzAO2V+o4c/g=; b=VNpu7vrJPoD1pTxOcGWVi4y0QqRg1zV8+4MZTKaB1Xi148ZfjysR51yAX/JLaexpPb D3/2Z8z5NUbG5gGzXjvIj6oLZoUZdQ1DNBnJBatQZNQi4y0frT1IxTeabjB8Gnf74HSG nTJxzqrv93gRJJ7ykibSyQ4KLSAC/CbK5LbwDvVMPjOcsVb6MCTydrm4H+8hN//N2MzC uAUxu+k8M9tZLo8m3PNAO92ZSqSqDAmj33jq+n2+d4X/6l5khxd00PsbaUt6EUYvS2wq T3mmBQ6NPh5o7sEOv6FmysBQRAs31x42qB3tWGX6L/QjKvp8tK3opE4/Y+2kgovFLFAm wQxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725917356; x=1726522156; 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=PWEX4r7jK/WQUNLo3UpE/qaecNNCCFHIzAO2V+o4c/g=; b=rWACleJCQZcYt4wJA24B31/4DWKmtIND6odwufPrIxjg3A/LBiZ5prQFRbhQKBeefR sfliM7uuhh7U8t1DmioBIWZ/u7RgxDT7OAR70f9r/IU4LxG4Xb/4VGERYbkAkMF3rmq0 ByOFSF4I71O9hDxy6TfO48CSljIs3nQNTl0qoe8G6DSK9WvC73twX/1DDr7QU5XR62GM ab1UakGMEkFiNrWm9KEKq21cgq8ShZeJYgn1OYqanooVxA6stFed+CVNCFHjonm/lHCz olR8mnTTeejgxdMLJ5v6gGy0NNxsKRA2+CpGYAsyg3m+M34W+eApmA2s2bzcQWyhnfNR WOiA== X-Forwarded-Encrypted: i=1; AJvYcCVquuN2J62uDBQ1w47bA/Dn6YMCUo2NtN73lmL44YdTVonmBvWbLKVxvI5yVW1jZae8Ucyvh8JKIQ==@kvack.org X-Gm-Message-State: AOJu0YyqJKIg4q7aUkC7sVV56pkzttu05cuHO4mYcBY+chLNpK1juLK2 ACE2PxIKAvqnOXqRyEs0zR5b2q2F+86vy2hbHymnBpd2nGhlgc5e6TEHzQmBhr7JpxfnMbtMbuW kZ1ouChFmKopELAiMsNQoX0STmMI= X-Google-Smtp-Source: AGHT+IHfUYAO11AnWDKGYgcX7wXzZr4/bp0e2f3bLddaNkIUpBnk0YADB/36ETv0KpYGTGDAp3Yj81Vw4zn5LqPQIhU= X-Received: by 2002:a17:902:dad1:b0:202:54b8:72e5 with SMTP id d9443c01a7336-206f0534658mr139583215ad.22.1725917356118; Mon, 09 Sep 2024 14:29:16 -0700 (PDT) MIME-Version: 1.0 References: <20240906051205.530219-1-andrii@kernel.org> <20240906051205.530219-3-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Mon, 9 Sep 2024 14:29:03 -0700 Message-ID: Subject: Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution To: Jann Horn , surenb@google.com, Liam Howlett Cc: Andrii Nakryiko , brauner@kernel.org, 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 923154000B X-Stat-Signature: gk1zgycdkcwafhf6wtszixjt3yhshsux X-Rspam-User: X-HE-Tag: 1725917357-909772 X-HE-Meta: U2FsdGVkX19KhpLR8zlXDFA/NCBKxQz7bQb5NKWnAm+q3csZGG+htwzXjtPs1OsWNYsWTJieqYIiYaoHjgi2tHLwOvekjzQkHsUyFmHcJcviwlb3Cy+mBVBRh6lIds46N4KnP90qP8A1/AITqUpn/ClCdl8iULsYIBlzkjZ1sa6x+P5ai6gfxAjwoUIsjGiknpaMySuO8CvRAEv+hY07MlyVU3UtVRvnIqSv55SOulzMFEA3SnU4FX+D1ptBZJyUbOF5jNor287kqmRoMviLTSxEynYduQkNhcCYokZVg/BvKjIMYlHxlWiN/mtZKqmGgiKLzogGBLPZvMLXYWZzf0nerEmADXYXoZY6LhROXdKKBLn15n2gzB9rr7jx3hPSYtbR83bRUdXbvWIiaXjjth08B7H3TjARbwgFV6LIjGBvQY07wJaCceqywR3ekXVHisESVbvq28ElvcWnMVNWSZUusqp2Z1RyMyM9O2iLf7FbA6rdQe4kp9wk/CSslm/IuUJYbB5IUKKU+V/BXto99qz1nDzmXOlZC6mocs0VkYduhkenSyNTCK8/bChwmEGOyf4b+9qQQ2pyaJPmVUtlDxX6yUg6s2yypWptfpjCNhx9nxW8WjC8n0hlgQ3+JvPNhSAfvIEA+WQ7mOSYrGB4Vt6iAFVlFjW1D21o9FsIsFTOYKLJ5HbM0QnNyKuVZyyIytEJkvTK+TVcMfjUXz3T18qv6qgva6x1VDxUF6Q5BpzAP8wUGRWWb+I+yeN0cBtVgsmHDaJQt/nIMOi7O7fiUJ1LFjKRQgFEAwr7T/wtifQxmIVrXyGpcEYVdt1Gkese4axChNHaFPSv3Rht640SXHBkC81drjd10aIkcuGqBk6avMrMD6aLA4eXBa56EV9Ms9cJDrOkwKHoeJeH1QcTY41o/H7xD34yH/imRBTasOGs839Se86aLVJUstyhLe1qGTswR1XwzHtFk42Yivo OG/X6bth 2AQLISWMQpJ6x2txhYyCo3CwhGXS9y9eqlzeE8R2ohvMMuvaKEAo4tp+2lKSc5/Qfve7XeIkjoe1fjFuAsRwC39QoUEiKOBahn40qedOVP9M+x4IQ5g55Gidw/tz7ax1KGyCPUwuOTqoEE/xEc1aHRy1qNaVgqHrAP1Bnc6lhAUwX/hUmJcGl1YaSdk47dONG2kBS0S8vm2vn4vTwgkeknk1jtNNr3nzBIcbmuUIRnN0hJSCo2LWtNWaQCAvslFjgQD7kW9VaaqHaypCm+8DMqp2p85jNxTSDpcsd+FQTQu3DUCn6dnCgv3X4hsvzMqwZGTDWkqQrg4n23CP+lbXAylxDglsRptPVOzab5Lz4+AdJmfJhqTAnKSa+5drt/+jEtLz0Rk0/l062RC+XMeH80NIsOQ== 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 Mon, Sep 9, 2024 at 6:13=E2=80=AFAM Jann Horn wrote: > > 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 b= e > * 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 del= ay. Good catch on FMODE_BACKING, I didn't realize there is this exception, than= ks! 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). > > 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 =3D VM_HUGETLB | VM_MAYEXEC | VM_MAYSHAR= E; > > + 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()). 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) !=3D VM_MAYEXEC) > > + goto bail; > > missing data_race() annotation on the vma->vm_flags access ack > > > + 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. > > yeah, let's leave it as a very unfortunate plan B and try to solve it a bit cleaner. > > > + 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_st= art); > > + 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; > > +}