Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
To:     Baoquan He <bhe@redhat.com>
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
In-Reply-To: <Y2zROzfhdR5UU+lv@MiWiFi-R3L-srv>
References: <20221109033535.269229-1-bhe@redhat.com>
 <20221109033535.269229-4-bhe@redhat.com> <87v8nnfwud.fsf@oracle.com>
 <Y2zROzfhdR5UU+lv@MiWiFi-R3L-srv>
Date:   Thu, 10 Nov 2022 10:48:01 -0800
Message-ID: <87mt8yfxy6.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?HWSkG+q/qZvEpjmx3ebmZplWwhOK3mLg7J2OYEHgQDseQGiiZEKHGfe9Hfyy?=
 =?us-ascii?Q?if9cKsQKDrBMH3fJCJIUdQnN8CV86ba7Nza4tQdG2/Ajro5XSZ+KDoZ54oCV?=
 =?us-ascii?Q?Q+guJq7wVIf7cqE6OY1yFPwyv06G056eNQNODAGqsMgNN67WkO3qIVnR/Tns?=
 =?us-ascii?Q?53kDalDPpJdq/9dFaGQZVaucYZ4yFOYUK6gVaMJtVDIMw005LFFm9xDEL2u5?=
 =?us-ascii?Q?hLmT0a3th1OOg/TqWkJFSaLI32Qxdf1wwyAY/KsLe9txBLItLgsxVKfeQyiL?=
 =?us-ascii?Q?px7N9f4w1XJvmSe+MbjC6FMrWxhM+tCmuxFZ+Zmfdwe7+F6NQktkXx4FLduw?=
 =?us-ascii?Q?JiHThvs8r7NL0bQMWnCMVdXZViHzyEB/bdIlW5R/DS2KPqhoKI3e7Hwwyd6j?=
 =?us-ascii?Q?KCWWccr2lhjMRoxKdjT4twILtstTxCYZ5nl2t3O6ecoi4Bgr9JxHJcbhN8MN?=
 =?us-ascii?Q?E16N/87wTQxXZKaCeiCTkaGN8bBtKxz2ewS65a3hBe6un991x+gRg3CCEbQT?=
 =?us-ascii?Q?im8DrHK1lWsSEU9f/FP7Jshy/uDlCj3Le6ifF9bUZSdfHwVZzgIwzNflQ8F7?=
 =?us-ascii?Q?D1/Z8K87j5T9S14NMmj2aNGa1NVr0/1u4a6A0tWdH+osssUE3zdIe7X7BWpY?=
 =?us-ascii?Q?kEOsch6tNWzIldrMRXxQb3PlUueg7WxNAvh61kaqmae5fMMZY5TYivO6iUNr?=
 =?us-ascii?Q?U+ey4+L+aCvZvjQezcSaSz4NRj/b6dKfRsvHNTXo70n0zjQ4VlHUx+Nz4M78?=
 =?us-ascii?Q?4eeF+CuNpvKI82zpo5u6lGosVsNfZof4oQyK9BmunEiDOFe0YYwx4nb6bqxg?=
 =?us-ascii?Q?nxprctjvQzp1tF3LMNygsLG5JxCFtX9pVb1gtmkPc89CPRIX992KbtCbOJqN?=
 =?us-ascii?Q?GwvpQFOnS6Jh+lkgdBPH6XGNbEz/coUPQEi0hVfZ6kZWeH4xH3/miYtgC0tU?=
 =?us-ascii?Q?Y4WnhbIFyrJlHy52jndU64KcoM91dMKWPErAgxrXa3cE0g2L1j1Aj+pHj5fX?=
 =?us-ascii?Q?kQsEIy+21iw+6Rw1mmGB6imLCEdSq+zyp+8FLK+slKuujZviKey2tOvhfhdN?=
 =?us-ascii?Q?4QLYJAshqMpqKdpbmZ1AWDRF7+99hC+Umbg8a9mH3rz9d8GpTx7ICNor5ASH?=
 =?us-ascii?Q?lAFSluOyR96cqT7CqPPUSbL8OpmYvfOUKPyB5ujdvuNGyParWPH1vrWnxUey?=
 =?us-ascii?Q?kfPi+ZM3CS1JpNfpRetNu890X1CbGv6/qYzeYplNyIHm/frPe15vZaxhYt10?=
 =?us-ascii?Q?QZhDhPrK0Dka0N6jeNk/9B2N9hf8DhB24ozEwv4h+WSpuu9J6jWlbzyjKtlZ?=
 =?us-ascii?Q?uB7luwsljYEx0q3OPC9toh87lJwmCBJ7omZqrUtyIc/POZfUJ+xXn1CSgcgB?=
 =?us-ascii?Q?d7+SMC8xJq8xAC+dYqxgLKOwY2c18KqPkoqo8Qkc8Xnml5ESR8dDJ0lYyNFW?=
 =?us-ascii?Q?aFMU+cIjSps/JNyG/LORS84WzUOvK+d6LAFz5nh199i1WTLUlZV8GEbEt7HY?=
 =?us-ascii?Q?Wu2jX2YO38UMZ8viWW0H7D02RsZiebIOkLnIfvzkQg+TBFNpIy0Xp5RF8cu6?=
 =?us-ascii?Q?iGj2D6ZtTrbHCUJC7Mt0pi4FUmKz1YkXqfDnDkjOUuYJlWYBMrgCgAuF8Wfo?=
 =?us-ascii?Q?rg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 45f1867e-3709-4ef7-b99a-08dac34c197d
X-MS-Exchange-CrossTenant-AuthSource: MN2PR10MB4173.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Nov 2022 18:48:04.1302
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Dq085gjTIZfIEVCkeJ0R+c+loGtjcwrzQLKZ1UrHcH7/fvnf/5az9oTVruO5SxMdT/qYRMaX0m4p7+M4YEaloRGSXdiEr1pT9vqHRMVVr6g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5180
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-10_12,2022-11-09_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0 mlxscore=0
 suspectscore=0 spamscore=0 malwarescore=0 bulkscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211100131
X-Proofpoint-ORIG-GUID: j6us02zbuAQWW0-qgXWSp_4OfCnxD7v3
X-Proofpoint-GUID: j6us02zbuAQWW0-qgXWSp_4OfCnxD7v3
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Baoquan He <bhe@redhat.com> 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.

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?

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;