On 02.07.2025 12:09, Jan Beulich wrote:
> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>> @@ -613,3 +612,91 @@ void __iomem *ioremap(paddr_t pa, size_t len)
>> {
>> return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
>> }
>> +
>> +int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
>> +{
>> + ASSERT_UNREACHABLE();
>> +
>> + return 0;
>> +}
>> +
>> +static struct domain *page_get_owner_and_nr_reference(struct page_info
>> *page,
>> + unsigned long nr)
>> +{
>> + unsigned long x, y = page->count_info;
>> + struct domain *owner;
>> +
>> + /* Restrict nr to avoid "double" overflow */
>> + if ( nr >= PGC_count_mask )
>> + {
>> + ASSERT_UNREACHABLE();
>> + return NULL;
>> + }
>
> I question the validity of this, already in the Arm original: I can't spot
> how the caller guarantees to stay below that limit. Without such an
> (attempted) guarantee, ASSERT_UNREACHABLE() is wrong to use. All I can see
> is process_shm_node() incrementing shmem_extra[].nr_shm_borrowers, without
> any limit check.
>
>> + do {
>> + x = y;
>> + /*
>> + * Count == 0: Page is not allocated, so we cannot take a
>> reference.
>> + * Count == -1: Reference count would wrap, which is invalid.
>> + */
>
> May I once again ask that you look carefully at comments (as much as at code)
> you copy. Clearly this comment wasn't properly updated when the bumping by 1
> was changed to bumping by nr.
>
>> + if ( unlikely(((x + nr) & PGC_count_mask) <= nr) )
>> + return NULL;
>> + }
>> + while ( (y = cmpxchg(&page->count_info, x, x + nr)) != x );
>> +
>> + owner = page_get_owner(page);
>> + ASSERT(owner);
>> +
>> + return owner;
>> +}
There also looks to be a dead code concern here (towards the "nr" parameters
here and elsewhere, when STATIC_SHM=n). Just that apparently we decided to
leave out Misra rule 2.2 entirely.
Jan
