linux-kernel - Re: [PATCH] xarray: Add a BUG_ON() to ensure caller is not sibling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <EF500105-614C-4D06-BE7A-AFB8C855BC78@nvidia.com>
Date: Mon, 02 Jun 2025 11:03:59 -0400
From: Zi Yan <ziy@...dia.com>
To: Dev Jain <dev.jain@....com>
Cc: akpm@...ux-foundation.org, willy@...radead.org,
 linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, david@...hat.com, anshuman.khandual@....com,
 ryan.roberts@....com
Subject: Re: [PATCH] xarray: Add a BUG_ON() to ensure caller is not sibling

On 29 May 2025, at 23:44, Dev Jain wrote:

> On 30/05/25 4:17 am, Zi Yan wrote:
>> On 28 May 2025, at 23:17, Dev Jain wrote:
>>
>>> On 28/05/25 10:42 pm, Zi Yan wrote:
>>>> On 28 May 2025, at 7:31, Dev Jain wrote:
>>>>
>>>>> Suppose xas is pointing somewhere near the end of the multi-entry batch.
>>>>> Then it may happen that the computed slot already falls beyond the batch,
>>>>> thus breaking the loop due to !xa_is_sibling(), and computing the wrong
>>>>> order. Thus ensure that the caller is aware of this by triggering a BUG
>>>>> when the entry is a sibling entry.
>>>> Is it possible to add a test case in lib/test_xarray.c for this?
>>>> You can compile the tests with “make -C tools/testing/radix-tree”
>>>> and run “./tools/testing/radix-tree/xarray”.
>>>
>>> Sorry forgot to Cc you.
>>> I can surely do that later, but does this patch look fine?
>> I am not sure the exact situation you are describing, so I asked you
>> to write a test case to demonstrate the issue. :)
>
>
> Suppose we have a shift-6 node having an order-9 entry => 8 - 1 = 7 siblings,
> so assume the slots are at offset 0 till 7 in this node. If xas->xa_offset is 6,
> then the code will compute order as 1 + xas->xa_node->shift = 7. So I mean to
> say that the order computation must start from the beginning of the multi-slot
> entries, that is, the non-sibling entry.

Got it. Thanks for the explanation. It will be great to add this explanation
to the commit log.

I also notice that in the comment of xas_get_order() it says
“Called after xas_load()” and xas_load() returns NULL or an internal
entry for a sibling. So caller is responsible to make sure xas is not pointing
to a sibling entry. It is good to have a check here.

In terms of the patch, we are moving away from BUG()/BUG_ON(), so I wonder
if there is a less disruptive way of handling this. Something like return
-EINVAL instead with modified function comments and adding a comment
at the return -EIVAL saying something like caller needs to pass
a non-sibling entry.

Best Regards,
Yan, Zi