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 D2F0FC4332F for ; Thu, 10 Nov 2022 10:24:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3590C8E0001; Thu, 10 Nov 2022 05:24:06 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 308DC6B0075; Thu, 10 Nov 2022 05:24:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1F7708E0001; Thu, 10 Nov 2022 05:24:06 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 1109E6B0074 for ; Thu, 10 Nov 2022 05:24:06 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id DAFBD161590 for ; Thu, 10 Nov 2022 10:24:05 +0000 (UTC) X-FDA: 80117147250.09.B9FD6CC Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf21.hostedemail.com (Postfix) with ESMTP id A917F1C0009 for ; Thu, 10 Nov 2022 10:24:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668075844; 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=czPIFT3o5TugDKi+RX7yE7CkBCTDZutrGeVsIeUfGZ0=; b=GIZ25Ek2aDUd2EWQiLTSJKLDDf+d3Hcw9KZI+mGupSKhkBYel/4+B4T2pEqi8ktXuyT188 PP0Kc7H6T+bULn+HvCB0XBMwMFowW80F5EbVn/rHYZ/tT9UUN/y2vC7QSh1cNDV+/81XDY 9Dge4H6bHDSwWESbsjji/kkiqyp/7sQ= 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-263-TpJWhQ4jOuqxEJmzFQ7N0Q-1; Thu, 10 Nov 2022 05:24:00 -0500 X-MC-Unique: TpJWhQ4jOuqxEJmzFQ7N0Q-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2708F800B30; Thu, 10 Nov 2022 10:24:00 +0000 (UTC) Received: from localhost (ovpn-12-86.pek2.redhat.com [10.72.12.86]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 671DD112131B; Thu, 10 Nov 2022 10:23:58 +0000 (UTC) Date: Thu, 10 Nov 2022 18:23:55 +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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87v8nnfwud.fsf@oracle.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=GIZ25Ek2; spf=pass (imf21.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.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=1668075845; a=rsa-sha256; cv=none; b=ICQKU2FmVwQCrLEg9qsZZVQIG4uLOKdHOEFIwKtIu8/DzXy2rK0LfmR14aIlysazhg2swb L/rTXyKxgwrriaL0Wq/POxv3qKvIN4piHti8RV6uQjy/hjLr2diA1muHtsUKaedbjzxbC5 5SvXllUVwnD/vGTf5dX/IdgEtlnGDFY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1668075845; 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=czPIFT3o5TugDKi+RX7yE7CkBCTDZutrGeVsIeUfGZ0=; b=MhQDEBoMsNY6HD2onDVVvm6c7qUBGNnSxeXnTyAuoKIqJljLrjpr7d1Whuq8wX/TbA7mW+ gDNgZufazdfTNJpweVCTr91yyhaKkc8RCUAgpxKzlW6dYqfuC2bv2WsQ4oSZH0jxcaTxw/ 18ZrISX0g1y6djkCaFfRxrHT7Hp0Snk= X-Stat-Signature: 9ebkzfzpgjr6ne8e3c7yjpgm4mwhdtfw X-Rspamd-Queue-Id: A917F1C0009 Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=GIZ25Ek2; spf=pass (imf21.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Rspam-User: X-Rspamd-Server: rspam06 X-HE-Tag: 1668075844-293748 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/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? diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 5a8d5659bfb0..78cae59170d8 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1890,6 +1890,7 @@ struct vmap_area *find_vmap_area(unsigned long addr) #define VMAP_RAM 0x1 #define VMAP_BLOCK 0x2 +#define VMAP_FLAGS_MASK 0x3 struct vmap_block_queue { spinlock_t lock; @@ -3588,7 +3589,7 @@ long vread(char *buf, char *addr, unsigned long count) struct vm_struct *vm; char *vaddr, *buf_start = buf; unsigned long buflen = count; - unsigned long n, size; + unsigned long n, size, flags; addr = kasan_reset_tag(addr); @@ -3609,12 +3610,14 @@ long vread(char *buf, char *addr, unsigned long count) if (!count) break; - if (!(va->flags & VMAP_RAM) && !va->vm) + if (!va->vm) continue; + flags = va->flags & VMAP_FLAGS_MASK; vm = va->vm; + vaddr = (char *) va->va_start; - size = vm ? get_vm_area_size(vm) : va_size(va); + size = flags ? va_size(va) : get_vm_area_size(vm); if (addr >= vaddr + size) continue; @@ -3630,9 +3633,9 @@ long vread(char *buf, char *addr, unsigned long count) if (n > count) n = count; - if ((va->flags & (VMAP_RAM|VMAP_BLOCK)) == (VMAP_RAM|VMAP_BLOCK)) + if ((flags & (VMAP_RAM|VMAP_BLOCK)) == (VMAP_RAM|VMAP_BLOCK)) vb_vread(buf, addr, n); - else if ((va->flags & VMAP_RAM) || !(vm->flags & VM_IOREMAP)) + else if ((flags & VMAP_RAM) || !(vm->flags & VM_IOREMAP)) aligned_vread(buf, addr, n); else /* IOREMAP area is treated as memory hole */ memset(buf, 0, n);