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 72A09C52D71 for ; Tue, 6 Aug 2024 16:14:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E74996B0085; Tue, 6 Aug 2024 12:14:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E24516B0088; Tue, 6 Aug 2024 12:14:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CEC466B0089; Tue, 6 Aug 2024 12:14:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id AE51C6B0085 for ; Tue, 6 Aug 2024 12:14:02 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 4217D80387 for ; Tue, 6 Aug 2024 16:14:02 +0000 (UTC) X-FDA: 82422317124.12.AB730E0 Received: from mail-yb1-f172.google.com (mail-yb1-f172.google.com [209.85.219.172]) by imf10.hostedemail.com (Postfix) with ESMTP id 73718C002B for ; Tue, 6 Aug 2024 16:14:00 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=MZtpf5gM; spf=pass (imf10.hostedemail.com: domain of surenb@google.com designates 209.85.219.172 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722960808; a=rsa-sha256; cv=none; b=tWGxZ2m7YJ3tx/dJ7esqirTdUlZpkqahPmFuaygsbrWjxB2e2IWpDr4nyzEfsdG4Fq4e9i RHWsbzVAtaQ05MHX4SpmJgsvdIfErAmfLTX9CafmxaU5Fo2K53+cNV/1fupmPm87Wy/iJv EWncCV0Gj7HxatN1+0xSRr5fxl5WUQM= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=MZtpf5gM; spf=pass (imf10.hostedemail.com: domain of surenb@google.com designates 209.85.219.172 as permitted sender) smtp.mailfrom=surenb@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=1722960808; 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=flHyanDSyOW3E9RQeJ9pSCPkgNyeeM0BzJyFB+YwtSQ=; b=gOGELsCXuHRThtOBue5Z/xA7JO4enEBPmEKxqY8cAkR8g99W1bTbSsQpvFR8hheTsbXCaH KkAh/ipAtiQXp03mZO/MGeR61c4Y7dFpRUFbOq/aug+XFPiLaZlTpjaQ3AFzp4O5Yuwoq5 nBpfbaJboqj0g60ivLAEq/SfRkLYTMo= Received: by mail-yb1-f172.google.com with SMTP id 3f1490d57ef6-e0bc6614d6cso787897276.2 for ; Tue, 06 Aug 2024 09:14:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1722960839; x=1723565639; 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=flHyanDSyOW3E9RQeJ9pSCPkgNyeeM0BzJyFB+YwtSQ=; b=MZtpf5gMSoM8Y7Ql+fIpQ/eOIdhtGdg3FEcvaw3/HVG3V0OxGRfGfYUysP1MsprbtR 256Gb6DfNm7pdqTfj2mX/2LB/igIdgdLKzrQ9d8aYFmoKLtFu7Ld2jtOeuYMzOzbXCcO MaAC2pz12dntdo9xhr1NB/I9YHnhtmLO/v93OFa/yuPqembyS2Hw96KejW2uoyTHAYEi SN5eheiKOSW5Fjz8fZIzKpQDJmy1PX6Jh8uQAAS04loj6g4AzcH+VVJtneIWSIigpRtf +4lOWMqDfq3XgZd6P4VseQgZBDNl9+AFr12HZSv3XIeZ055E+Xej6vpk1nxIy8dtQ3Ls HW5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722960839; x=1723565639; 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=flHyanDSyOW3E9RQeJ9pSCPkgNyeeM0BzJyFB+YwtSQ=; b=parxATcgSD4L72QLdvyrSdNjIiz0UXhMkKFb4xsDDMMoTzzCequQTII2Uvgf5p4TBE CZzzC1XbKoCX/QoubiqJS9zR1bQjT62EsIJbRHEhS75oKQ+a+7W/PDS46jVCEFeKHGdI p3pTYwuJ2XBgKPsfHTZfU3FzLohhSpNX2IqT//vnx+3ZQqkuHLCwbWTUjex0TsmkMabn W1t3rwDoY+9VWNdce3MWToDYgen3H9A7IICbRf8Arkyo2I+BCND2W3wqD3ssKqHtYTLo PdI15UCPVtD5kJC5oJp5TqfkltJareHssnhnc1fSzRsqODwlEhuDTJj7Id1+Xki1KToY hYuA== X-Forwarded-Encrypted: i=1; AJvYcCVXf6hcrvca1gmOF82r7Mn+pAAW0L23LexlDJBr3r4/U8Hjjy4AGUf6W4fpjdZxGN4j+pOP1csOGBUObJ4qOr1bpTs= X-Gm-Message-State: AOJu0YwNqr6mvrtHluCqUCwKPXoNFYDRFxaqnvoPVixhzJnlTRpEAB9t mEr2xSkEbmVBxi8LdwKSwli4wSoY8jdmpw+evvRAaQz8T2A3K9ZYxkDPXfFkEEl3oLk4TaYzYPq 7DHlylDUgTZVdJ+a/tQkDH44IU0ZLn92R97dp X-Google-Smtp-Source: AGHT+IFIR7KB3qC95IwE99Ecy1X9S4TKWbi7I5lgIk7hVv/EXJ2eNsSamr8zi6SVuWSu0NNUtVbJv6C43PAZ39oOiJ8= X-Received: by 2002:a25:918a:0:b0:e0b:c402:b03f with SMTP id 3f1490d57ef6-e0bde34c22dmr12985019276.27.1722960838924; Tue, 06 Aug 2024 09:13:58 -0700 (PDT) MIME-Version: 1.0 References: <20240805-fix-vma-lock-type-confusion-v1-1-9f25443a9a71@google.com> In-Reply-To: <20240805-fix-vma-lock-type-confusion-v1-1-9f25443a9a71@google.com> From: Suren Baghdasaryan Date: Tue, 6 Aug 2024 09:13:45 -0700 Message-ID: Subject: Re: [PATCH] mm: fix (harmless) type confusion in lock_vma_under_rcu() To: Jann Horn Cc: Andrew Morton , Matthew Wilcox , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: 1szomiwuqkec8hk4acafzfy1fmee6bq9 X-Rspamd-Queue-Id: 73718C002B X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1722960840-212482 X-HE-Meta: U2FsdGVkX1/y1ZUbnPTxaebohMioBFyZZhOhuvo9OGQzg7efGWgiqeodRxPOkUT2ERASIIVYy2pu/XJfJh3xJPzbOzZM67ScOUs0vkquF4PIVh0VNiqEndy7zqi/+83Y0KthHk1rd5S4x/7vSaFJ2e/6F5tBxNhY8XmLkQTkDowCQl4siuDZDRHCTU7WLu2BCyaIG1M0UR0cRC+oUvDlodGNuByYiYyeBGzLKw0pca7+uX06UozoLhfbXxZ9nFWAs0DF4Tw0FHM3+7r6qWZYazvmvm7quaqznP7ZfXdZHZlSBCr135v7xWw9D1KS1vXNPf+26d9VH2h8MkkX+swqJlC1eC+9+Im9RCOzrQekkIfZf2LPPWsOsXFy1IrhhidAwUsHhWz0toX2BVdcUhdW6BpNWXQu6fnM9dE/wwKseG2L+W8Z6h5sbT9YFKnI/oeR5IijlQUjxV086N0JiHjotQe0jQ4MO/707ADw+Fvs8+sEOE5su1UE0B0cCA+nV1SM5z8lKHNmmF2VtY9p3d6VGl+Zw7mqU6z3mBlB1wLKQqAqNFPTFCQwbgPaZ8/aUeob4t/lfBTFqZEoRwBpZyRtOoVqnWqolNbKwAJBL+AR+LG/HBtfncDgrln5kILPgqIT+VNXrQjsB4klEZks+3hTSxEv5El9hpff+vGScgD8tjnm7p0iMw2p025i4+4UQioqzGyKsMPDMlbq6P4xHTUPWhL4qw9wvg2jl4DXp7rbLx+/nRka4SkuQnBe2JG17b/8wjq+YrNG1d+Vjw9IHYPKKceDogqTlkJYUPgwAEDOOqMlzaQ2iclcK+10uibGNgkVH31s/b/M1IQ2RDH8+ME/tG98V39lb2UWJlkwY2SYEOG8Bee+fy1DMY2FpNhbVIqNdhqyo6Kb8H6tCFu8V630gezpLp48e1HTMIzzYxdMvGDjqZbHPLsSTor9+IK/ce81l6F/Z5ALHE/KFSWcaTk UADCxxIg Hwm/F3ubrWNUKvZ/8EgdJzEYOHWLqKZ5xg51tNPvMKotREjjoDYe9wWCP3xIMmf9HF2aRdyVuhgblCJetOaIlSjl/Dsiu2NIIuqaRMpOPRhZgbzPNQeLF9fSKR7LaACGjG8H4fqIKrXBxqQt4t2nVSAaq+FKQp9nnCNy3oms7/7FwbGv9Vgz+bHmDfolwTohnZhng2W3vdspiJd24wr0/3NEICGpO4S2a2G2E4oEsGq/hPrI= 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, Aug 5, 2024 at 5:52=E2=80=AFAM Jann Horn wrote: > > There is a (harmless) type confusion in lock_vma_under_rcu(): > After vma_start_read(), we have taken the VMA lock but don't know yet > whether the VMA has already been detached and scheduled for RCU freeing. > At this point, ->vm_start and ->vm_end are accessed. > > vm_area_struct contains a union such that ->vm_rcu uses the same memory a= s > ->vm_start and ->vm_end; so accessing ->vm_start and ->vm_end of a detach= ed > VMA is illegal and leads to type confusion between union members. > > Fix it by reordering the vma->detached check above the address checks, an= d > document the rules for RCU readers accessing VMAs. > > This will probably change the number of observed VMA_LOCK_MISS events > (since previously, trying to access a detached VMA whose ->vm_rcu has bee= n > scheduled would bail out when checking the fault address against the > rcu_head members reinterpreted as VMA bounds). > > Fixes: 50ee32537206 ("mm: introduce lock_vma_under_rcu to be used from ar= ch-specific code") > Signed-off-by: Jann Horn Thanks for fixing this subtle issue and clearly documenting the rules! Not sure if we should CC stable? It is harmless but it's still a bug... Acked-by: Suren Baghdasaryan > --- > sidenote: I'm not entirely sure why we handle detached VMAs and moved > VMAs differently here (detached VMAs retry, moved VMAs bail out), but > that's kinda out of scope of this patch. Yeah, technically in both cases the address space is being modified and we should bail out and retry with mmap_lock. I just think if the VMA got replaced while we are calling lock_vma_under_rcu(), it's reasonable to retry the search and try finding the new VMA if it's already established and unlocked. > --- > include/linux/mm_types.h | 15 +++++++++++++-- > mm/memory.c | 14 ++++++++++---- > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 485424979254..498cdf3121d0 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -657,58 +657,69 @@ struct vma_numab_state { > > /* > * This struct describes a virtual memory area. There is one of these > * per VM-area/task. A VM area is any part of the process virtual memory > * space that has a special rule for the page-fault handlers (ie a share= d > * library, the executable area etc). > + * > + * Only explicitly marked struct members may be accessed by RCU readers = before > + * getting a stable reference. > */ > struct vm_area_struct { > /* The first cache line has the info for VMA tree walking. */ > > union { > struct { > /* VMA covers [vm_start; vm_end) addresses within= mm */ > unsigned long vm_start; > unsigned long vm_end; > }; > #ifdef CONFIG_PER_VMA_LOCK > struct rcu_head vm_rcu; /* Used for deferred freeing. */ > #endif > }; > > - struct mm_struct *vm_mm; /* The address space we belong to= . */ > + /* > + * The address space we belong to. > + * Unstable RCU readers are allowed to read this. > + */ > + struct mm_struct *vm_mm; > pgprot_t vm_page_prot; /* Access permissions of this VMA= . */ > > /* > * Flags, see mm.h. > * To modify use vm_flags_{init|reset|set|clear|mod} functions. > */ > union { > const vm_flags_t vm_flags; > vm_flags_t __private __vm_flags; > }; > > #ifdef CONFIG_PER_VMA_LOCK > - /* Flag to indicate areas detached from the mm->mm_mt tree */ > + /* > + * Flag to indicate areas detached from the mm->mm_mt tree. > + * Unstable RCU readers are allowed to read this. > + */ > bool detached; > > /* > * Can only be written (using WRITE_ONCE()) while holding both: > * - mmap_lock (in write mode) > * - vm_lock->lock (in write mode) > * Can be read reliably while holding one of: > * - mmap_lock (in read or write mode) > * - vm_lock->lock (in read or write mode) > * Can be read unreliably (using READ_ONCE()) for pessimistic bai= lout > * while holding nothing (except RCU to keep the VMA struct alloc= ated). > * > * This sequence counter is explicitly allowed to overflow; seque= nce > * counter reuse can only lead to occasional unnecessary use of t= he > * slowpath. > */ > int vm_lock_seq; > + /* Unstable RCU readers are allowed to read this. */ > struct vma_lock *vm_lock; > #endif > > /* > * For areas with an address space and backing store, > * linkage into the address_space->i_mmap interval tree. > diff --git a/mm/memory.c b/mm/memory.c > index 34f8402d2046..3f4232b985a1 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -5996,23 +5996,29 @@ struct vm_area_struct *lock_vma_under_rcu(struct = mm_struct *mm, > if (!vma) > goto inval; > > if (!vma_start_read(vma)) > goto inval; > > - /* Check since vm_start/vm_end might change before we lock the VM= A */ > - if (unlikely(address < vma->vm_start || address >=3D vma->vm_end)= ) > - goto inval_end_read; > - > /* Check if the VMA got isolated after we found it */ > if (vma->detached) { > vma_end_read(vma); > count_vm_vma_lock_event(VMA_LOCK_MISS); > /* The area was replaced with another one */ > goto retry; > } > + /* > + * At this point, we have a stable reference to a VMA: The VMA is > + * locked and we know it hasn't already been isolated. > + * From here on, we can access the VMA without worrying about whi= ch > + * fields are accessible for RCU readers. > + */ > + > + /* Check since vm_start/vm_end might change before we lock the VM= A */ > + if (unlikely(address < vma->vm_start || address >=3D vma->vm_end)= ) > + goto inval_end_read; > > rcu_read_unlock(); > return vma; > > inval_end_read: > vma_end_read(vma); > > --- > base-commit: de9c2c66ad8e787abec7c9d7eff4f8c3cdd28aed > change-id: 20240805-fix-vma-lock-type-confusion-0a956d9d31ae > -- > Jann Horn >