Re: [PATCH 07/13] s390/idals: Use kmalloc() for IDAL data buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Replacing __get_free_pages() with kmalloc() violates the strict 4K page-alignment requirement for s390 Indirect Data Address Words (IDAWs), leading to hardware I/O rejection and out-of-bounds memory access.
- [Medium] Kmemleak will falsely report IDAL data chunks as memory leaks because only their physical addresses are stored.

Pre-existing issues:
- [High] Unprotected concurrent modification of device->char_data.ibs in tape_check_idalbuffer() leading to a Use-After-Free.
--

commit ee00e66345d4d1d6997d0f2f7bda11ae7ad10516
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

s390/idals: Use kmalloc() for IDAL data buffers

idal_buffer_alloc() allocates the data chunks of an IDAL buffer that is
used for channel I/O.
These buffers can be allocated with kmalloc() as there's nothing special
about them to go directly to the page allocator.

Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com

> diff --git a/arch/s390/include/asm/idals.h b/arch/s390/include/asm/idals.h
> index 06e1ec2afd5af..248829d461bce 100644
> --- a/arch/s390/include/asm/idals.h
> +++ b/arch/s390/include/asm/idals.h
> @@ -147,7 +147,7 @@ static inline struct idal_buffer *idal_buffer_alloc(size_t size, int page_order)
>  			ib->data[i] = dma64_add(ib->data[i - 1], IDA_BLOCK_SIZE);
>  			continue;
>  		}
> -		vaddr = (void *)__get_free_pages(GFP_KERNEL, page_order);
> +		vaddr = kmalloc(PAGE_SIZE << page_order, GFP_KERNEL);

[Severity: High]
Does replacing __get_free_pages() with kmalloc() violate the strict 4K
page-alignment requirement for s390 Indirect Data Address Words (IDAWs)?

kmalloc() with memory debugging enabled (like SLUB redzoning) can return
unaligned addresses for PAGE_SIZE allocations. Since the s390 Channel
Subsystem mandates that second and subsequent IDAWs must be aligned to a
4K boundary, could this unaligned address cause hardware I/O rejection?

Additionally, if the base address is unaligned, does adding 4K via
dma64_add() result in out-of-bounds array access, since the nr_ptrs
calculation mathematically assumes a 4K-aligned start address?

>  		if (!vaddr)
>  			goto error;
>  		ib->data[i] = virt_to_dma64(vaddr);

[Severity: Medium]
Will kmemleak falsely report these IDAL data chunks as memory leaks?

Since virt_to_dma64() stores the physical address, the kmemleak scanner
won't find the exact virtual address bit pattern returned by kmalloc()
during its garbage collection scan.

[ ... ]

This is a pre-existing issue, but I noticed a potential race condition in
the tape driver when checking IDAL buffers.

In drivers/s390/char/tape_char.c:tapechar_read():

    rc = tape_check_idalbuffer(device, block_size);

And in drivers/s390/char/tape_core.c:tape_check_idalbuffer():

    idal_buffer_array_free(&device->char_data.ibs);
    device->char_data.ibs = new;

[Severity: High]
If two threads call read() or write() concurrently without device->mutex
being held, could one thread free device->char_data.ibs while the other
is executing tape_do_io() or iterating over the old ibs array? Can this
lead to a use-after-free or data corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-s390-cio-ready-v1-0-ca7f39806234@kernel.org?part=7