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 72278EDE99A for ; Tue, 10 Sep 2024 16:32:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EF1528D0096; Tue, 10 Sep 2024 12:32:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E79658D0002; Tue, 10 Sep 2024 12:32:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CF3708D0096; Tue, 10 Sep 2024 12:32:25 -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 AF18C8D0002 for ; Tue, 10 Sep 2024 12:32:25 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 61BFD1A0841 for ; Tue, 10 Sep 2024 16:32:25 +0000 (UTC) X-FDA: 82549371450.21.CE8591E Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf26.hostedemail.com (Postfix) with ESMTP id 7EE7F14000F for ; Tue, 10 Sep 2024 16:32:22 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=u2E581Jq; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf26.hostedemail.com: domain of surenb@google.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725985866; a=rsa-sha256; cv=none; b=lWp7kDPdDH2XFzdHC7j5lSUVjFkxtMgdJPef+ULMnX4lBfB3/NU+xiRV4OIZcVqW1+x2ld WtpYpCdvV7AlQFmuAESXCgR9n2PAsdWiyyCIY5YxPCnxrcGipWjklKvsE0zKj1C/t5+IJR rBg5xlQ8oOe15uVc0zNAk+JhlEOOmXg= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=u2E581Jq; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf26.hostedemail.com: domain of surenb@google.com designates 209.85.160.182 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725985866; 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=f9ncZWKOVJEsq4iDNvlY1nT7hu4+4z6i+c0WR3K9aTs=; b=AXX148H71cQzrVWEuMDapDxewrsaUEBTDA76HRUQHYHl+rz/BLDLRn/2D07vX1KtDH2UJF 5o7KQafy1URiwCqCnbnYPzOmaeSpyFYiqNJKdf2IalPO1Fx9GxTJTQ5ia0RMEDC2wQ1WoK MlUVDdtD2DdWivIEuDEqiNz52lHCy/o= Received: by mail-qt1-f182.google.com with SMTP id d75a77b69052e-4582fa01090so317711cf.0 for ; Tue, 10 Sep 2024 09:32:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1725985941; x=1726590741; 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=f9ncZWKOVJEsq4iDNvlY1nT7hu4+4z6i+c0WR3K9aTs=; b=u2E581Jq/n+zCINxVETuM0eZiP48ntddHOCwDiUAQyK/D+YdB3LalVAGCufUVogWa5 smduKM4w26Kj8IWJs0PR/aYjEU1QIMvEkoJFBCr7z6I+magw6ZMdeJQ+r8v80/v0imV2 XcKkYzDoOQGP/JEGr7KsNKwPrXRQhJlxzc2z6UpY/I00+tP7no2SozQBoE4UEhb9g6Mt aiX68Iy7s3clcowx9a9vRt7nCYaLy5HRLejvzv7wZeXLRIpMrHFU3WT9B+12WjnuiAzq wBUmwpGScjOGfE7sQY6UHMUqnZAyuPpcQExr8xRi/eYQ/VNCRA4Q7f6eH1YtfGyu58BK eeJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725985941; x=1726590741; 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=f9ncZWKOVJEsq4iDNvlY1nT7hu4+4z6i+c0WR3K9aTs=; b=ecVgxzXqmxuPZwVFKQ4aArAEf5PVKNJOTL9yMnLFqXgB1RmD9jPJbXvVo59p8DTK3s EYnp+47UE7S81VtqIlIrJFMlDTPfEUBYBrGXz3rdpUHh+Ml77oXZCZ84l1hZgE3NMCiB gFOBqW30nxVuH6UD4j8AnNsA2La1flANyUCgDRk7tuz0uNFsszMtV8f4AVJHarroQaeR 3DTO8k+hsQjJaEW115Ta8mupivAgZrjxYjbbx3bapHCiVH1z9+PyhFk5OUmC2rz4GMv/ KtYirL9CQA94zLh3Wbv+Dpr/iaspqgsGah2izmBeonLx+NcXmz7+JmDyF3H6YcWN/ErF M8uQ== X-Forwarded-Encrypted: i=1; AJvYcCX6S5sTvaRK38cnpu0EIQuh2aScJNqxX73WesHA/zgHxYG685aBLyWW/Tt+J4KA10rUrjKAGlQWgg==@kvack.org X-Gm-Message-State: AOJu0YyfsGJlVP3KbZaYfIXcvRQMHzNhYgvns49N+Eg4ciTm9I6cESE2 nJkEuuAjUoundvd68y8s8ZziWb4mx/lLNp7HxVPfwv/g2RKSgSyNvk9JRNC0B56WokOoxrN+Xcj UNllXvv2LqrEGXUZ1msPFOymekxu0CMtEtorL X-Google-Smtp-Source: AGHT+IGzGAumkHrr2bDnkqRNDINvDYeTyLaFVCvxRDWY8NaodXVwMYUWwwjnEltyVzzl/ubtMR1u/3thAFVQbJZiYv0= X-Received: by 2002:a05:622a:11c8:b0:456:7228:c0a2 with SMTP id d75a77b69052e-4583efc8c1fmr3705951cf.13.1725985941140; Tue, 10 Sep 2024 09:32:21 -0700 (PDT) MIME-Version: 1.0 References: <20240906051205.530219-1-andrii@kernel.org> <20240906051205.530219-3-andrii@kernel.org> In-Reply-To: From: Suren Baghdasaryan Date: Tue, 10 Sep 2024 09:32:08 -0700 Message-ID: Subject: Re: [PATCH 2/2] uprobes: add speculative lockless VMA-to-inode-to-uprobe resolution To: Andrii Nakryiko , Christian Brauner Cc: 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 7EE7F14000F X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: rw5u1hqek4zmzpugtswaamwek14gprak X-HE-Tag: 1725985942-758858 X-HE-Meta: U2FsdGVkX1+Y1Hv35hLsVOccwmRDSWZPM+XOixitw7lr6yxNwIsKHKSZdaCtKxlKtuGEdNdOt9hTv5CTM9wkcfD6XWeStiiYA5phKbxfjXr+yrkXyObrZyyIXFey9EqKuUMc2ac7nh4EkSICoXev6xYFLp46esXrEjKLrS/GrILEJI0R++T5uBg3CEVJA1MkAqVu/QTBioQ5iPLasjoS8ir14DS2uYuEIsKjATjjhd8/seLDpI14zP/wNs0k7apy/ol5ENjwy8pwrG9Uy7m0H79Tr3KYEH+eDBPcn+AwGYv7TT6f7v7Rh4qP/VWeuF1x/M0N8hbLz6awOFAmabNCqiLH7wjZs/+VGJFD+Fp7PlOVIxd2FMWBqpwVp4Ud2zZOMXE8pHmko4pC8QL3TY7DUB9Atibe7TRRxoRC1xrtAzAgnVHERVH6q/UD1HZvECz45OZmXHKmmqKZFSH9LLJXIkIRxWRgbSAHqHp2ir4SmBZPbAK9tQmFdEfGh7B6FO4U4eKEdPLInT/69L/lpx5mXNRdkwT7/hzGvB1XUquNaSCGIBWh6srv3UjDXnpgJ2l2q2xgRUGqC0ji7wSdXldgbZIz9Pl2GFrwALA+ilA/UOEsA2wJQ8muauSYYW6llbRbZH+1TbI/xUx9jT+5CmNV1u4UX1OahCYyWTe+o6g9bXnC4uy4kSXYao+bla4b4+uFHrlTLpseZfNKF93dQybPAzzP1nh2qNmEMNVEZZCQJdwLDPlEyTnBAt9MTbiNvP0n7rEQU25SPdtTEZW1xqxQGLkSTFU+EF5W02WebxkTyCscANO1gxM0ONk7IBVfmIxxjLrclk35nK1PZ9IWOU3JNTxRUFJPjMgjoMRGdWvYCyliI9BsF6EKstKyjUghiXZfkM7HVp+qWKCKYTtTu0ztUnZY4Z9s8zGHtFsCkXWCjpa7F+7k/32Y3jyaYr+c53/hjmCtqv8iQsOujrhRmmI DYTA+Dmj aCg0I3yEL8mSXjd4Z14SYZ0KKnte9paBM7jMCqCghqa/ubdb/E6Bc2aXL3O4jMYj7fv8aslATvufzF/81w5NEPaA5j/SgiYmMkKQOwJwhMCfak1FZ3b6JPt+AatXGT9Ywgz2ovfrh2Wkr+DkeHs6QK3fhhUqv1Kz1TsnTQ1GPd775Sqp8qFrnEqQcz1YjiEcf5p+VBCpmJtcoxXcsguRZ0CRTNrdNW0Qhq1hnLmxKCIEcD+92SYu3STZ1pR5E1dzQDomLF1JQvbSU/K6+ydOTVWeI96B0ZBHoT/IafXaReEEHj/HDNtvvYDX6nC32fAJ3Ncipub9BD6guNtw8K9+Ypho1uA== 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 2:29=E2=80=AFPM Andrii Nakryiko wrote: > > 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 safe= ly > > > access vma->vm_file->f_inode field locklessly under just rcu_read_loc= k() > > > > 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 d= elay. > > Good catch on FMODE_BACKING, I didn't realize there is this exception, th= anks! > > 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 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@bra= uner/ > > > > > 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 b= p_vaddr) > > > +{ > > > + const vm_flags_t flags =3D VM_HUGETLB | VM_MAYEXEC | VM_MAYSH= ARE; > > > + 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_= start); > > > + 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; > > > +}