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 634EEC4332F for ; Mon, 14 Nov 2022 10:07:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DFC566B0072; Mon, 14 Nov 2022 05:07:06 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D84956B0073; Mon, 14 Nov 2022 05:07:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BFE2F6B0074; Mon, 14 Nov 2022 05:07:06 -0500 (EST) 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 AC12F6B0072 for ; Mon, 14 Nov 2022 05:07:06 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 6953C1C3DAC for ; Mon, 14 Nov 2022 10:07:06 +0000 (UTC) X-FDA: 80131619652.08.4817F13 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf20.hostedemail.com (Postfix) with ESMTP id 055D21C0009 for ; Mon, 14 Nov 2022 10:07:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668420425; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=YOB5ovzwG41qBNYEX8G5z12o/BuLOtHt6/b8j0c3Fwg=; b=ausWuoWcvvrqBdOmoYSjiw5e1MTuloMEi1be4Get4bVKHXZV++Ijxhp3Qro1In307/zYkM fO2EtNvi59woUJQVekl3sdJtEdQyEZqScaGub8LJjyxTFNbMT+QGzd/irf+NMbxwH1YpC4 5bxfhW0+u0Gt/jQN2A4cnQQq4TtIlZg= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-310-Aw8vjTctObut8IMzRN_KUg-1; Mon, 14 Nov 2022 05:06:54 -0500 X-MC-Unique: Aw8vjTctObut8IMzRN_KUg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B74EE101A54E; Mon, 14 Nov 2022 10:06:53 +0000 (UTC) Received: from localhost (ovpn-12-71.pek2.redhat.com [10.72.12.71]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E57E71731B; Mon, 14 Nov 2022 10:06:52 +0000 (UTC) Date: Mon, 14 Nov 2022 18:06:49 +0800 From: Baoquan He To: Stephen Brennan Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, urezki@gmail.com, hch@infradead.org Subject: Re: [PATCH RFC 3/3] mm/vmalloc.c: allow vread() to read out vm_map_ram areas Message-ID: References: <20221109033535.269229-1-bhe@redhat.com> <20221109033535.269229-4-bhe@redhat.com> <87v8nnfwud.fsf@oracle.com> <87mt8yfxy6.fsf@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87mt8yfxy6.fsf@oracle.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1668420426; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=YOB5ovzwG41qBNYEX8G5z12o/BuLOtHt6/b8j0c3Fwg=; b=paXJtqk4N4RdUR3MqnXylutfAt2tEiE3Krgbhl/G6zWiUs24cuRaLV1Xl74/73pATOIyF4 I/+bigWSyr0edPrdSyBrYikvZKJm+nncFKdUeRUI35+DifWsQN3fz/atyKAkIaFXuFfvGK 3TzW3VX0gyKAvrSLHfIoFIr0zzoxhtw= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=ausWuoWc; spf=pass (imf20.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1668420426; a=rsa-sha256; cv=none; b=Od/f1fIiCNfl7OFn1L+jOswzybXr6tVxnYGIhz/R6YoKtsKGmlMFnCGwH5X7TRh1M2Q+hh 6oFdWihF91ExHRAAeyjaETfHbw5cgccTUhtgJUS6JqZTtPwt1BA8BHIeCDoDtc8dcetvqs AJ7Ru2/KzjYf8Qg9Bk/u27+o/bN45CI= X-Stat-Signature: dzb53jzerd7du9j7hymcowgk6quw8mmn X-Rspamd-Queue-Id: 055D21C0009 Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=ausWuoWc; spf=pass (imf20.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Rspamd-Server: rspam07 X-Rspam-User: X-HE-Tag: 1668420425-190722 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: On 11/10/22 at 10:48am, Stephen Brennan wrote: > Baoquan He writes: > > > On 11/09/22 at 04:59pm, Stephen Brennan wrote: > > ...... > >> > @@ -3569,12 +3609,14 @@ long vread(char *buf, char *addr, unsigned long count) > >> > if (!count) > >> > break; > >> > > >> > - if (!va->vm) > >> > + if (!(va->flags & VMAP_RAM) && !va->vm) > >> > continue; > >> > > >> > vm = va->vm; > >> > - vaddr = (char *) vm->addr; > >> > - if (addr >= vaddr + get_vm_area_size(vm)) > >> > + vaddr = (char *) va->va_start; > >> > + size = vm ? get_vm_area_size(vm) : va_size(va); > >> > >> Hi Baoquan, > >> > >> Thanks for working on this. I tested your patches out by using drgn to > >> debug /proc/kcore. I have a kernel module[1] to do a vm_map_ram() call > >> and print the virtual address to the kernel log so I can try to read > >> that memory address in drgn. When I did this test, I got a panic on the > >> above line of code. > > ...... > >> Since flags is in a union, it shadows "vm" and causes the condition to > >> be true, and then get_vm_area_size() tries to follow the pointer defined > >> by flags. I'm not sure if the fix is to have flags be a separate field > >> inside vmap_area, or to have more careful handling in the vread path. > > > > Sorry, my bad. Thanks for testing this and catching the error, Stephen. > > > > About the fix, both way are fine to me. I made a draft fix based on the > > current patchset. To me, adding flags in a separate field makes code > > easier, but cost extra memory. I will see what other people say about > > this, firstly if the solution is acceptable, then reusing the union > > field or adding anohter flags. > > > > Could you try below code to see if it works? > > I tried the patch below and it worked for me: reading from vm_map_ram() > regions in drgn was fine, gave me the correct values, and I also tested > reads which overlapped the beginning and end of the region. Thanks a lot for the testing. > > That said (and I don't know the vmalloc code well at all), I wonder > whether you can be confident that the lower 2 bits of the va->vm pointer > are always clear? It looks like it comes from kmalloc, and so it should > be aligned, but can slab red zones mess up that alignment? Hmm, it should be OK. I am also worried about the other places of va->vm checking. I will check code again to see if there's risk in the case you mentioned. I may change to add another ->flags field into vmap_area in v2 post. > > I also tested out this patch on top of yours, to be a bit more cautious. > I think we can be confident that the remaining bits are zero when used > as flags, and non-zero when used as a pointer, so you can test them to > avoid any overlap. But it's probably too cautious. > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 78cae59170d8..911974f32b23 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -3613,7 +3613,7 @@ long vread(char *buf, char *addr, unsigned long count) > if (!va->vm) > continue; > > - flags = va->flags & VMAP_FLAGS_MASK; > + flags = (va->flags & ~VMAP_FLAGS_MASK) ? 0 : (va->flags & VMAP_FLAGS_MASK); > vm = va->vm; > > vaddr = (char *) va->va_start; >