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 B3F65D3C92D for ; Sat, 19 Oct 2024 21:17:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 06AFA6B007B; Sat, 19 Oct 2024 17:17:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 01B1A6B0083; Sat, 19 Oct 2024 17:17:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DFDAC6B0085; Sat, 19 Oct 2024 17:17:51 -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 C11B36B007B for ; Sat, 19 Oct 2024 17:17:51 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id A3F1CC0E1E for ; Sat, 19 Oct 2024 21:17:36 +0000 (UTC) X-FDA: 82691613312.07.601E134 Received: from mail-pl1-f179.google.com (mail-pl1-f179.google.com [209.85.214.179]) by imf06.hostedemail.com (Postfix) with ESMTP id 1D03B18000F for ; Sat, 19 Oct 2024 21:17:39 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=XUxj+hv7; spf=pass (imf06.hostedemail.com: domain of surenb@google.com designates 209.85.214.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=1729372521; 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=lG9RUbcTNoGLIzLhj402kfJLkduFmo4+SdSyItO45us=; b=xsS0aG+W/iCskJi3GkslHKvyl/bFzcf7fRdCHtIdBkpU+ZGzseEzNJfnzBsZePkA0SIN4j E2Zv2WpXgsYRAPkjR9wxD/LIOy1dwmQ+5kwBjAsuKppJRksljxqCV6M0NhmFXNHRKCp22f InFBifrDxlsmCaa095Vo7nRraai6T5s= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729372521; a=rsa-sha256; cv=none; b=MlhvO6v5X48E9hPy8v39/AMKL4XYvuna9tHOSZjkoQmlVaEfj1T/IRV121MIOaqC7AMS1p Rz0HQpbS0vkV7QGvCt7HPuw2CdpsBGP7I04BbAxTKOzznGkRnmnyhGAn27c/5d4z6/h1Ls u02HAs2Yxe6f3io4eXm4E3D9Y7JijXA= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=XUxj+hv7; spf=pass (imf06.hostedemail.com: domain of surenb@google.com designates 209.85.214.179 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-pl1-f179.google.com with SMTP id d9443c01a7336-20ca4877690so101505ad.1 for ; Sat, 19 Oct 2024 14:17:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1729372668; x=1729977468; 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=lG9RUbcTNoGLIzLhj402kfJLkduFmo4+SdSyItO45us=; b=XUxj+hv7lDoG8YZ9n5+vDbhYfJx1oCLxOUAv+3PK07Zo3SNdSV51CJE7hgy/BYHJgQ fWNHQSZ+JiQIDWvkMP5F/Ri6qctVafywS6E7OcodxCRLF88MIE/VU29aGcFULNrQ1gYy xiAzoQgUrbrRioMzjBSJE7IEo8AslL+Fw9PMe2OrpLn88NaJjlyC76czM5SXYOQWE6Da vlDrfElHrPTa8OX+R0fRE6NKm4AGa41kQVlNmVIgIYcGjOHTXdSukfMTqgPaTkice9zc g2F/nZAalzKTQWA+Ly6QrDALWVh9rdG+wl5dV4m0z/+5CMSXLeMqYXrjTXB++YzY/Wpv xAOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729372668; x=1729977468; 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=lG9RUbcTNoGLIzLhj402kfJLkduFmo4+SdSyItO45us=; b=KY1nzjdws9DsSDfN1kJToDcI/YAoToqyvA5U2BhwLhJx2///c6aZCh/76zOwxX++C8 BXbed+SRArSjL7raHkJdB17JPwCIIz39nBZi756NZr+Wjv6ISzIQjfv31Hdm6vEAvib0 POLk4Cm1EyYw5KhuuWLQXuy3626SqGUcsnpmvPMKlkEai2IpT9ERTyWoz1ilGTaKZ+jF WvK2stayvIhJyA3GfzLESNLPkpWFupiZrTPizOhzWMgjseT76aZ9yvMaBL0fKMg5nhJV +Cv8ZX1jfavx31vm69WfHFt8DcDeow0xPThwz9/XhlLXAxptNbyt7kD+aKNAbH9QrIWJ sgSw== X-Forwarded-Encrypted: i=1; AJvYcCVCdEgZi1/TYDdKd2LqKjeHpGKx/gJ0X3G4QzKPUXaEi9xvcvG1lhwNiIMu4FhfEdppgxZPb/4k4g==@kvack.org X-Gm-Message-State: AOJu0YwdoDVXpjA+W+sWLipewDMIwUN85Zt1ZLegEMUmmYvvwPkYEaAk yzOl75DA3ygkkFJkaLG7j4KSnxP4TeVpKmb9o/kwsqvQzqeLtZzSWwTDj4zXrbJUh7l4930Wcp2 3xtrjyLZ0PiBFr1FRLMzP+4pbLNhSPLuhGCpS X-Google-Smtp-Source: AGHT+IHguZVmRVYbxE+VGmkLQoGSkVCZz6trpqohu9rlENnnGaoCSdxxoYOGaNY1ZeTeTvTl18sghl3wfdZFn/rJM7I= X-Received: by 2002:a17:902:c411:b0:20c:5fbe:4e66 with SMTP id d9443c01a7336-20e742bc45bmr1425025ad.27.1729372666765; Sat, 19 Oct 2024 14:17:46 -0700 (PDT) MIME-Version: 1.0 References: <20241018152925.138341-1-hao.ge@linux.dev> <20241018162559.143548-1-hao.ge@linux.dev> <2f771446-be6a-3b7c-4595-be8c8188145a@linux.dev> In-Reply-To: <2f771446-be6a-3b7c-4595-be8c8188145a@linux.dev> From: Suren Baghdasaryan Date: Sat, 19 Oct 2024 14:17:35 -0700 Message-ID: Subject: Re: [PATCH v2] mm/codetag: move ref and tag null pointer check to alloc_tag_add To: Hao Ge Cc: kent.overstreet@linux.dev, akpm@linux-foundation.org, yuzhao@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Hao Ge Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 1D03B18000F X-Stat-Signature: hhmkg6tkoamqubifzbz9ew7cyqwr51dp X-HE-Tag: 1729372659-374620 X-HE-Meta: U2FsdGVkX1+aS6y3hhPzDVX/jAmJskV/VMCcPMQp+zfglN/KYJJHgHXoRhNhMNbegRQ+ChTx7rnh8cGSEdgglKHGqT0uwzf6cvk/dSQFlTbuYDhRH31sIvhvKvjxdFfwhq0DwZmSujXME2664pw6hA2tR5JfK1W3sDftac/i86LQHcn2dAH/5znFwt2LBNeIBsdpvtUTS/URSOa7VmxnGh8IKGage30/KLUOu0bcR+bOqPQFka6KBFj6GGmB+yDpBGqXlmk1MKdX5luvtS40zbaJKeIiEqG3Gz2eBegcAGVwXDkMvO9Y5S5aQduxx5KxYo331h9Asn6A0Pm29c8k7pqT9r5vYVT1fgDxaPNzbaMLL6zFyTQ0kATIeJq1acSnhqwbSAr/pWcnBZwvNymQRrcqLe+V9zelf8fx6hFdl0CK6LcMuRHEmowP0HjSe5P+v3P0cSUCq2JvT3E9oG/mT5D9DUjhRKuudoosNZT7ljXlSIjzVa18f/v/QWV4uSSByUaQTKdfy4/jUaEG9zht7+yoloUqa/qGdyFg9AgLSxay6VuVji630WzoNmzWSqBOBV1mZI7oc4+iB0dmPFSj2B00uKs/wSAAUYsLCd8SXigogmgG1eH3kTzXvZXxQp47z6C57U0osp0vJ6TEEw2Wfabw7peHO3VgH5gwQWbJNxS+v5ddCTSilxeFteyr10HvNekslLawt1t3lSkPO9IxFlrs+NfKYISkoMplx4WMCAJRvXmhx0pByceBGDnPtr8HYJQxFoQ428AAClemg7CGjI7ZKZlsu7qKchI35EsihQuFqRv97Fa5c1sjBtnYXABeP61m1Zqoae+o24bZG2nKyaVWVTCwBk8jm4W4aY66pjF/CUBRDwkoDpsQZU6e/2JynPSbXg0T7E09pVNZeWDEDPI2hQEu3xxpImRBJPOAerZhWoH3KrXTewOs6F2AuBrlffrR3Onj6LfM10y3SYB Z3QPENp2 eFaEi3QRr8ZxSL8+y4lMQOdDoJUdDC6Y3xU9jmR9fAYNl6k7FnH6cHdH307SUIejBCirYltvndim0XL1D6Ia5N5GOd+1xkwbfh32hn//dcTDGsPVqyUiUv4hJ7x2rx/5WwAOc2iNpuszyqDVVvDbBD+iFslLi3X3Pg7scHJ1/tNrSCbSvmSUFqoeDEDHvRjhL9kyFSMi0LwhypXFE9YavzeQw5+tR1a6aO1fusa2/bSmZsLA= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000003, 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 Fri, Oct 18, 2024 at 5:47=E2=80=AFPM Hao Ge wrote: > > Hi Suren > > > On 10/19/24 01:30, Suren Baghdasaryan wrote: > > On Fri, Oct 18, 2024 at 9:26=E2=80=AFAM Hao Ge wrote= : > >> From: Hao Ge > >> > >> When we compile and load lib/slub_kunit.c,it will cause a panic. > >> > >> The root cause is that __kmalloc_cache_noprof was directly called > >> instead of kmem_cache_alloc,which resulted in no alloc_tag being > >> allocated.This caused current->alloc_tag to be null,leading to a > >> null pointer dereference in alloc_tag_ref_set. > >> > >> Despite the fact that my colleague Pei Xiao will later fix the code > >> in slub_kunit.c,we still need to move the null pointer check for ref > >> and tag to alloc_tag_add here. > >> It is sufficient for us to issue a warning to the user; > >> It should not lead to a panic. > >> > >> Here is the log for the panic: > >> > >> [ 74.779373][ T2158] Unable to handle kernel NULL pointer dereferenc= e at virtual address 0000000000000020 > >> [ 74.780130][ T2158] Mem abort info: > >> [ 74.780406][ T2158] ESR =3D 0x0000000096000004 > >> [ 74.780756][ T2158] EC =3D 0x25: DABT (current EL), IL =3D 32 bit= s > >> [ 74.781225][ T2158] SET =3D 0, FnV =3D 0 > >> [ 74.781529][ T2158] EA =3D 0, S1PTW =3D 0 > >> [ 74.781836][ T2158] FSC =3D 0x04: level 0 translation fault > >> [ 74.782288][ T2158] Data abort info: > >> [ 74.782577][ T2158] ISV =3D 0, ISS =3D 0x00000004, ISS2 =3D 0x000= 00000 > >> [ 74.783068][ T2158] CM =3D 0, WnR =3D 0, TnD =3D 0, TagAccess =3D= 0 > >> [ 74.783533][ T2158] GCS =3D 0, Overlay =3D 0, DirtyBit =3D 0, Xs = =3D 0 > >> [ 74.784010][ T2158] user pgtable: 4k pages, 48-bit VAs, pgdp=3D0000= 000105f34000 > >> [ 74.784586][ T2158] [0000000000000020] pgd=3D0000000000000000, p4d= =3D0000000000000000 > >> [ 74.785293][ T2158] Internal error: Oops: 0000000096000004 [#1] SMP > >> [ 74.785805][ T2158] Modules linked in: slub_kunit kunit ip6t_rpfilt= er ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_conntrack ebtabl= e_nat ebtable_broute ip6table_nat ip6table_mangle 4 > >> [ 74.790661][ T2158] CPU: 0 UID: 0 PID: 2158 Comm: kunit_try_catch K= dump: loaded Tainted: G W N 6.12.0-rc3+ #2 > >> [ 74.791535][ T2158] Tainted: [W]=3DWARN, [N]=3DTEST > >> [ 74.791889][ T2158] Hardware name: QEMU KVM Virtual Machine, BIOS 0= .0.0 02/06/2015 > >> [ 74.792479][ T2158] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT= -SSBS BTYPE=3D--) > >> [ 74.793101][ T2158] pc : alloc_tagging_slab_alloc_hook+0x120/0x270 > >> [ 74.793607][ T2158] lr : alloc_tagging_slab_alloc_hook+0x120/0x270 > >> [ 74.794095][ T2158] sp : ffff800084d33cd0 > >> [ 74.794418][ T2158] x29: ffff800084d33cd0 x28: 0000000000000000 x27= : 0000000000000000 > >> [ 74.795095][ T2158] x26: 0000000000000000 x25: 0000000000000012 x24= : ffff80007b30e314 > >> [ 74.795822][ T2158] x23: ffff000390ff6f10 x22: 0000000000000000 x21= : 0000000000000088 > >> [ 74.796555][ T2158] x20: ffff000390285840 x19: fffffd7fc3ef7830 x18= : ffffffffffffffff > >> [ 74.797283][ T2158] x17: ffff8000800e63b4 x16: ffff80007b33afc4 x15= : ffff800081654c00 > >> [ 74.798011][ T2158] x14: 0000000000000000 x13: 205d383531325420 x12= : 5b5d383734363537 > >> [ 74.798744][ T2158] x11: ffff800084d337e0 x10: 000000000000005d x9 = : 00000000ffffffd0 > >> [ 74.799476][ T2158] x8 : 7f7f7f7f7f7f7f7f x7 : ffff80008219d188 x6 = : c0000000ffff7fff > >> [ 74.800206][ T2158] x5 : ffff0003fdbc9208 x4 : ffff800081edd188 x3 = : 0000000000000001 > >> [ 74.800932][ T2158] x2 : 0beaa6dee1ac5a00 x1 : 0beaa6dee1ac5a00 x0 = : ffff80037c2cb000 > >> [ 74.801656][ T2158] Call trace: > >> [ 74.801954][ T2158] alloc_tagging_slab_alloc_hook+0x120/0x270 > >> [ 74.802494][ T2158] __kmalloc_cache_noprof+0x148/0x33c > >> [ 74.802976][ T2158] test_kmalloc_redzone_access+0x4c/0x104 [slub_k= unit] > >> [ 74.803607][ T2158] kunit_try_run_case+0x70/0x17c [kunit] > >> [ 74.804124][ T2158] kunit_generic_run_threadfn_adapter+0x2c/0x4c [= kunit] > >> [ 74.804768][ T2158] kthread+0x10c/0x118 > >> [ 74.805141][ T2158] ret_from_fork+0x10/0x20 > >> [ 74.805540][ T2158] Code: b9400a80 11000400 b9000a80 97ffd858 (f940= 12d3) > >> [ 74.806176][ T2158] SMP: stopping secondary CPUs > >> [ 74.808130][ T2158] Starting crashdump kernel... > >> > >> Fixes: e0a955bf7f61 ("mm/codetag: add pgalloc_tag_copy()") > >> Signed-off-by: Hao Ge > >> --- > >> v2: Modify the errors in the title and commit message. > >> Remove the empty lines that were mistakenly added in version v1. > >> --- > >> include/linux/alloc_tag.h | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h > >> index 1f0a9ff23a2c..8603e3a9df10 100644 > >> --- a/include/linux/alloc_tag.h > >> +++ b/include/linux/alloc_tag.h > >> @@ -137,10 +137,6 @@ static inline void alloc_tag_sub_check(union code= tag_ref *ref) {} > >> /* Caller should verify both ref and tag to be valid */ > >> static inline void __alloc_tag_ref_set(union codetag_ref *ref, struc= t alloc_tag *tag) > >> { > >> - alloc_tag_add_check(ref, tag); > >> - if (!ref || !tag) > >> - return; > >> - > > Unfortunately this change will result in __alloc_tag_ref_set() and > > alloc_tag_ref_set() missing the following important check from > > alloc_tag_sub_check(): > > > > Maybe I missed something here, I'm a bit confused. Can you give me an > example to explain it? Sure. The warning I indicated would trigger when (ref !=3D NULL && ref->ct !=3D NULL), which means that we have a valid tag reference and it's pointing to a valid tag. During allocation that is abnormal because we expect that if the reference is valid (ref !=3D NULL), it should have been cleared (ref->ct =3D=3D NULL). With your change we won't get these warnings when calling __alloc_tag_ref_set() and alloc_tag_ref_set(). Does that make sense? > > Thanks > > > Best regards > Hao > > > > WARN_ONCE(ref && ref->ct, > > "alloc_tag was not cleared (got tag for %s:%u)\n", > > ref->ct->filename, ref->ct->lineno); > > > > I think the change below would fix this issue without the above > > mentioned side-effect: > > > > -static inline void __alloc_tag_ref_set(union codetag_ref *ref, struct > > alloc_tag *tag) > > +static inline bool __alloc_tag_ref_set(union codetag_ref *ref, struct > > alloc_tag *tag) > > { > > alloc_tag_add_check(ref, tag); > > if (!ref || !tag) > > - return; > > + return false; > > > > ref->ct =3D &tag->ct; > > + return true; > > } > > > > -static inline void alloc_tag_ref_set(union codetag_ref *ref, struct > > alloc_tag *tag) > > +static inline bool alloc_tag_ref_set(union codetag_ref *ref, struct > > alloc_tag *tag) > > { > > - __alloc_tag_ref_set(ref, tag); > > + if (unlikely(!__alloc_tag_ref_set(ref, tag))) > > + return false; > > + > > /* > > * We need in increment the call counter every time we have a = new > > * allocation or when we split a large allocation into smaller= ones. > > * Each new reference for every sub-allocation needs to increm= ent call > > * counter because when we free each part the counter will be > > decremented. > > */ > > this_cpu_inc(tag->counters->calls); > > + return true; > > } > > > > static inline void alloc_tag_add(union codetag_ref *ref, struct > > alloc_tag *tag, size_t bytes) > > { > > - alloc_tag_ref_set(ref, tag); > > - this_cpu_add(tag->counters->bytes, bytes); > > + if (likely(alloc_tag_ref_set(ref, tag))) > > + this_cpu_add(tag->counters->bytes, bytes); > > } > > > > Could you please confirm this fix? > > Thanks, > > Suren. > > > >> ref->ct =3D &tag->ct; > >> } > >> > >> @@ -158,6 +154,10 @@ static inline void alloc_tag_ref_set(union codeta= g_ref *ref, struct alloc_tag *t > >> > >> static inline void alloc_tag_add(union codetag_ref *ref, struct allo= c_tag *tag, size_t bytes) > >> { > >> + alloc_tag_add_check(ref, tag); > >> + if (!ref || !tag) > >> + return; > >> + > >> alloc_tag_ref_set(ref, tag); > >> this_cpu_add(tag->counters->bytes, bytes); > >> } > >> -- > >> 2.25.1 > >>