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 3547DC77B7C for ; Mon, 23 Jun 2025 15:39:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CDD638D0009; Mon, 23 Jun 2025 11:39:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C8E508D0005; Mon, 23 Jun 2025 11:39:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BCB9A8D0009; Mon, 23 Jun 2025 11:39:21 -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 AC79A8D0005 for ; Mon, 23 Jun 2025 11:39:21 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 50DA614045C for ; Mon, 23 Jun 2025 15:39:21 +0000 (UTC) X-FDA: 83587074522.17.A7D2696 Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by imf25.hostedemail.com (Postfix) with ESMTP id 69D01A000E for ; Mon, 23 Jun 2025 15:39:19 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=LezcNNGc; spf=pass (imf25.hostedemail.com: domain of surenb@google.com designates 209.85.160.179 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=1750693159; 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=yXpGhB8D/EFyEFVKBdpBHH5TY+f9AXT6r2w1zPMMC18=; b=pcoyYYPo/QtOF9kXlFotsgN3O6b87gsEi/XpgMHh560Bk/s2kIaKhOHyBfW8WDfbbnQAOb GB3I9YdBm6Mup18dM3wGEzbxoh5c7f3yQaPwjxyGmmRF0QOMfo2pGEPsakQvTwOW/HWImd fy8r7pWTk16v3U//icgfFskSjXDoyQs= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=LezcNNGc; spf=pass (imf25.hostedemail.com: domain of surenb@google.com designates 209.85.160.179 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=1750693159; a=rsa-sha256; cv=none; b=B+jEy7Yl1AXLDIKoFCCYS8vq3o6MQAaXAkwZSUJ82FOeCtZmnJ/RYnQKgrQHLzw+HqXk+N siHdxGTS/CRk60fW25p4g9n79IpyHvPhpGjMEzg/FLbPHDRg5xJKwqZzO3PGwEbjlyS4ah nDUqYRGgPachIcplUatyGhjiPX+ue24= Received: by mail-qt1-f179.google.com with SMTP id d75a77b69052e-47e9fea29easo757471cf.1 for ; Mon, 23 Jun 2025 08:39:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1750693158; x=1751297958; 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=yXpGhB8D/EFyEFVKBdpBHH5TY+f9AXT6r2w1zPMMC18=; b=LezcNNGc8wa8h0fqDQ2PSVM6JLNptCpxIUHRaLIxebAfk38RmwOGyTfWPjs71CAnkq n/+eQIJPwBHkDUQ6X1NnTEUz4a+/ikLwDaWrWXvY4vx8hFqWhc9L8Ipxqc65QTM1c3ZL 03tTaxtyh4fMNB9o7gb/tKep5Zi9MJHPry1sv66fDaYt6+Mc+N+EEI3K6lokLNQwzM9Q K/hmegbEI6RlvRQrGBJFog4IY3+oCKPFEzt0q0KlBBh7Blx5F/sZCRgKo/ahdAPyiYjt tHsvoMXxahvevTpljx49Aa8vT8z6CcBrNyHtJsjgCzcWQW8oV7Q+UPeQZl18LFVlqTn0 V0cg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750693158; x=1751297958; 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=yXpGhB8D/EFyEFVKBdpBHH5TY+f9AXT6r2w1zPMMC18=; b=HcZ4VA5Hq8Zd9LK1nS/Gm3IWxL8WqZo1EBCNcRZzjAbbMzX4GENsvyvzlBJdVTB/B9 EokNvvr92YjpDsPPi33r2AEecn/izDC32CeAIdefxFywsj6/uaXYDH7cmN6VAC2zlb+D 8RE9NLhCzjzSHkG2Qoi1+DWb/I4lzHa/zNlMZ0EmoQyCeVmgvOXVdxJHa85VDq1TSjx8 asGYCzmg3GIKPv62njAdlafPGIJj15OYVpBAq6gONqI2P5rUyF8A1vb5NFqQUGdS9drM g+RszU1RTsbPFcvCb6N9ERuS8cnV7ugmLDU2BmWZfEOkwzQ+49ZQM4to866k6qJlPqVo uoIg== X-Forwarded-Encrypted: i=1; AJvYcCVSdMNV3RwhvJaq2qSUEGPS9thiIFGL6jKFBno0ckkhWzXUDWKOCD4olOq9l3VVrteCwZYa87IE6Q==@kvack.org X-Gm-Message-State: AOJu0Yx5+bS8EHdiCGvIMLxiUV0WXXmsdPzVw91OFMzwKxGWCV+FJVIq tLX+9/hKTqxdLxDMl5HGq7TfHYsmaKNewbPXAf6CkE/Rp3pz3+N7MNh/KmlSJuJmQOanJykF97H G1M5pN18gE+Rpxt9z9NhR4mPA4diTcRJAusbOi8g9 X-Gm-Gg: ASbGncvx/rsoq0QIYyQE5lFBxtfmiL7TC5Kf1adj97INI9uN7pTQJwYPmjVvA2dZss5 lZ0F48CZ5SLCP1wWDE1YqbGBtK69J8pmsnDR0azG+3RDU0aGGEKLRdLCDR2I7ABkipF0Xs2r++u eCwstyKS6h6vdGgEi0iQHidrJFjleIgH3VnRoko+UfCPTNXOp+JAXK X-Google-Smtp-Source: AGHT+IF95EFLkc/FCP1YCcV3dI+eQE9bOqB4iNEeLrvaybuWX/WfpRtkSxzFcESxVTnkO2L/DcAlKqhu8ewdgrnOaN4= X-Received: by 2002:a05:622a:8596:b0:480:dde:aa4c with SMTP id d75a77b69052e-4a785418ccdmr6434191cf.4.1750693158070; Mon, 23 Jun 2025 08:39:18 -0700 (PDT) MIME-Version: 1.0 References: <20250623-anon_name_cleanup-v1-0-04c94384046f@suse.cz> <20250623-anon_name_cleanup-v1-1-04c94384046f@suse.cz> In-Reply-To: <20250623-anon_name_cleanup-v1-1-04c94384046f@suse.cz> From: Suren Baghdasaryan Date: Mon, 23 Jun 2025 08:39:07 -0700 X-Gm-Features: Ac12FXypuLgcEDBw9XhSelwuQgJJ0khGAjfDvtlFfTgvOwT9ulXoeO2zFqOjGy0 Message-ID: Subject: Re: [PATCH RFC 1/2] mm, madvise: simplify anon_name handling To: Vlastimil Babka Cc: Andrew Morton , "Liam R. Howlett" , Lorenzo Stoakes , David Hildenbrand , Jann Horn , Mike Rapoport , Michal Hocko , Colin Cross , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 69D01A000E X-Stat-Signature: a6zhqycsny6cpyw4yexpqfeupdh4jhne X-HE-Tag: 1750693159-633810 X-HE-Meta: U2FsdGVkX19lvlq1tD5VGXgvJrkC9uxKqWUX9meJHDUQEJKn1ELS/W6g9693BnzzXbs53VUxnzy7MFX4ueFP4Jg/MaRlSViEfAz7bVPxJP9ut8UhSwanRVANvAMxANCXESxzbjwNjcmU9yvIRpdgKaGVjf4ERWzkkt7a17CNIhEHiq30zZV9fuyA9Na/5jMuyLGRKv4TZkWDjxdp4qWm1rty76Jso1YUP6Oge5TjbfHDjbxcurcfWhs2qmT0DXQ6mPKW0EMvvy7xn29rOeLYyDI716w5PMNwuEyIUVrNXJcQ4J+oORh8QxJMJq/SqqzKaKxfnMmin0/uMaGbHLjQJEb0hYEijzoaXQLrYR/fMT1kXyXaPi89MjDei77xkWS32EItgbnJCJAFiR9Blsq49vkRLNhEBpRtFYTdW5/B7x+37doYJBpjZu1imI4Ni6zRPEiRHwveaMrRjhcicABUSYA4rgGFPRR/XEk80CRSINKO7XThhAofs2SBrqmW/sZKcSQExhMPMTv9Ybsj7TsaMmBZULbffc3Rd81+1nLy/g/N/vhmPFV00Lr79c6DJ5uXj7t4BAjv7ImXQuW6ffBgB3BhwbbvJNheYn7mmRtgTC+bCkK5veNMXTVGCoFwRF6T/I48HTGp1m66A1/nsqIgk0GyOA9Sc+KlkRkY2WL0PJ/40A3XFFGfimuEjrfBcagry9sZMK/f2fONm3K11MQG8nsYrhMaICqGow83VxZPu0czs5JUDqi83ErjRmpsrYHKvc1aPS906bkn/MsoasdyVpQzyafi9V8qQ95ObvBkUNpBWelQ0ny0Lu0x7I0GoC66XNUid9AFR159S787iFxHpLbSKhzlUN2jdHrmX2StPHAb4qpxLtNtgIC/bbaRU00lnCgWCxWCYOIRaJk/bC5bl8c1M7TsJ1wlMvxO4I6ipB+QQ38UqiZnOvB6dUwmo02rSejejTiC2ymPal1vjQh m0v2xSGo po7tR9lYETzIW5W+220lWTyuiZdn7y/zPL6Q++b7qCvTd0anasZ+iejsB90DWik3lo7AJJa1h57F8SjGrzlHsaqYefjaCEnuB6/V6UCli3S78iTpJGGWnerx24gy+DQ51mSFF6AK2rc5qsvZw+e6T2dNEWIvD2nbanIlGmQF5dIlAVEPt83Zu+emKBTVgLY3QjQ87WQUWKgfhJVH5yRpJaJkVb8FT+vGXvRGKY56xQCrNSO0CIFFnyZ/WE5VMNW+R8yaCHO7ov3G1KVU= 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, Jun 23, 2025 at 8:00=E2=80=AFAM Vlastimil Babka wr= ote: > > Since the introduction in 9a10064f5625 ("mm: add a field to store names > for private anonymous memory") the code to set anon_name on a vma has > been using madvise_update_vma() to call replace_vma_anon_name(). Since s/replace_vma_anon_name()/replace_anon_vma_name() > the former is called also by a number of other madvise behaviours that > do not set a new anon_name, they have been passing the existing > anon_name of the vma to make replace_vma_anon_name() a no-op. > > This is rather wasteful as it needs anon_vma_name_eq() to determine the > no-op situations, and checks for when replace_vma_anon_name() is allowed > (the vma is anon/shmem) duplicate the checks already done earlier in > madvise_vma_behavior(). It has also lead to commit 942341dcc574 ("mm: > fix use-after-free when anon vma name is used after vma is freed") > adding anon_name refcount get/put operations exactly to the cases that > actually do not change anon_name - just so the replace_vma_anon_name() > can keep safely determining it has nothing to do. > > The recent madvise cleanups made this suboptimal handling very obvious, > but happily also allow for an easy fix. madvise_update_vma() now has the > complete information whether it's been called to set a new anon_name, so > stop passing it the existing vma's name and doing the refcount get/put > in its only caller madvise_vma_behavior(). > > In madvise_update_vma() itself, limit calling of replace_anon_vma_name() > only to cases where we are setting a new name, otherwise we know it's a > no-op. We can rely solely on the __MADV_SET_ANON_VMA_NAME behaviour and > can remove the duplicate checks for vma being anon/shmem that were done > already in madvise_vma_behavior(). > > The remaining reason to obtain the vma's existing anon_name is to pass > it to vma_modify_flags_name() for the splitting and merging to work > properly. In case of merging, the vma might be freed along with the > anon_name, but madvise_update_vma() will not access it afterwards This is quite subtle. Can we add a comment in the code that anon_name might be freed as a result of vma merge after vma_modify_flags_name() gets called and anon_name should not be accessed afterwards? > so the > UAF previously fixed by commit 942341dcc574 is not reintroduced. > > Signed-off-by: Vlastimil Babka Reviewed-by: Suren Baghdasaryan > --- > mm/madvise.c | 37 +++++++++++++------------------------ > 1 file changed, 13 insertions(+), 24 deletions(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 4491bf080f55d6d1aeffb2ff0b8fdd28904af950..ae29395b4fc7f65a449c5772b= 1901a90f4195885 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -176,21 +176,25 @@ static int replace_anon_vma_name(struct vm_area_str= uct *vma, > } > #endif /* CONFIG_ANON_VMA_NAME */ > /* > - * Update the vm_flags on region of a vma, splitting it or merging it as > - * necessary. Must be called with mmap_lock held for writing; > - * Caller should ensure anon_name stability by raising its refcount even= when > - * anon_name belongs to a valid vma because this function might free tha= t vma. > + * Update the vm_flags and/or anon_name on region of a vma, splitting it= or > + * merging it as necessary. Must be called with mmap_lock held for writi= ng. > */ > static int madvise_update_vma(vm_flags_t new_flags, > struct madvise_behavior *madv_behavior) > { > - int error; > struct vm_area_struct *vma =3D madv_behavior->vma; > struct madvise_behavior_range *range =3D &madv_behavior->range; > - struct anon_vma_name *anon_name =3D madv_behavior->anon_name; > + bool set_new_anon_name =3D madv_behavior->behavior =3D=3D __MADV_= SET_ANON_VMA_NAME; > + struct anon_vma_name *anon_name; > VMA_ITERATOR(vmi, madv_behavior->mm, range->start); > > - if (new_flags =3D=3D vma->vm_flags && anon_vma_name_eq(anon_vma_n= ame(vma), anon_name)) > + if (set_new_anon_name) > + anon_name =3D madv_behavior->anon_name; > + else > + anon_name =3D anon_vma_name(vma); > + > + if (new_flags =3D=3D vma->vm_flags && (!set_new_anon_name > + || anon_vma_name_eq(anon_vma_name(vma), anon_name= ))) > return 0; > > vma =3D vma_modify_flags_name(&vmi, madv_behavior->prev, vma, Maybe here we can add a comment, something like this: /* * vma->anon_name might be freed by vma_modify_flags_name() as a result of vma merge, * therefore accessing anon_name in the code below is unsafe if !set_new_anon_name. */ > @@ -203,11 +207,8 @@ static int madvise_update_vma(vm_flags_t new_flags, > /* vm_flags is protected by the mmap_lock held in write mode. */ > vma_start_write(vma); > vm_flags_reset(vma, new_flags); > - if (!vma->vm_file || vma_is_anon_shmem(vma)) { > - error =3D replace_anon_vma_name(vma, anon_name); > - if (error) > - return error; > - } > + if (set_new_anon_name) > + return replace_anon_vma_name(vma, anon_name); > > return 0; > } > @@ -1313,7 +1314,6 @@ static int madvise_vma_behavior(struct madvise_beha= vior *madv_behavior) > int behavior =3D madv_behavior->behavior; > struct vm_area_struct *vma =3D madv_behavior->vma; > vm_flags_t new_flags =3D vma->vm_flags; > - bool set_new_anon_name =3D behavior =3D=3D __MADV_SET_ANON_VMA_NA= ME; > struct madvise_behavior_range *range =3D &madv_behavior->range; > int error; > > @@ -1403,18 +1403,7 @@ static int madvise_vma_behavior(struct madvise_beh= avior *madv_behavior) > /* This is a write operation.*/ > VM_WARN_ON_ONCE(madv_behavior->lock_mode !=3D MADVISE_MMAP_WRITE_= LOCK); > > - /* > - * madvise_update_vma() might cause a VMA merge which could put a= n > - * anon_vma_name, so we must hold an additional reference on the > - * anon_vma_name so it doesn't disappear from under us. > - */ > - if (!set_new_anon_name) { > - madv_behavior->anon_name =3D anon_vma_name(vma); > - anon_vma_name_get(madv_behavior->anon_name); > - } > error =3D madvise_update_vma(new_flags, madv_behavior); > - if (!set_new_anon_name) > - anon_vma_name_put(madv_behavior->anon_name); > out: > /* > * madvise() returns EAGAIN if kernel resources, such as > > -- > 2.50.0 >