Re: [PATCH 1/5] io_uring: Adjust mapping wrt architecture aliasing requirements

On 2023-07-12 12:24, Helge Deller wrote:
> Hi Matoro,
> 
> * matoro <[email protected]>:
>> On 2023-03-14 13:16, Jens Axboe wrote:
>> > From: Helge Deller <[email protected]>
>> >
>> > Some architectures have memory cache aliasing requirements (e.g. parisc)
>> > if memory is shared between userspace and kernel. This patch fixes the
>> > kernel to return an aliased address when asked by userspace via mmap().
>> >
>> > Signed-off-by: Helge Deller <[email protected]>
>> > Signed-off-by: Jens Axboe <[email protected]>
>> > ---
>> >  io_uring/io_uring.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 51 insertions(+)
>> >
>> > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> > index 722624b6d0dc..3adecebbac71 100644
>> > --- a/io_uring/io_uring.c
>> > +++ b/io_uring/io_uring.c
>> > @@ -72,6 +72,7 @@
>> >  #include <linux/io_uring.h>
>> >  #include <linux/audit.h>
>> >  #include <linux/security.h>
>> > +#include <asm/shmparam.h>
>> >
>> >  #define CREATE_TRACE_POINTS
>> >  #include <trace/events/io_uring.h>
>> > @@ -3317,6 +3318,54 @@ static __cold int io_uring_mmap(struct file
>> > *file, struct vm_area_struct *vma)
>> >  	return remap_pfn_range(vma, vma->vm_start, pfn, sz,
>> > vma->vm_page_prot);
>> >  }
>> >
>> > +static unsigned long io_uring_mmu_get_unmapped_area(struct file *filp,
>> > +			unsigned long addr, unsigned long len,
>> > +			unsigned long pgoff, unsigned long flags)
>> > +{
>> > +	const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
>> > +	struct vm_unmapped_area_info info;
>> > +	void *ptr;
>> > +
>> > +	/*
>> > +	 * Do not allow to map to user-provided address to avoid breaking the
>> > +	 * aliasing rules. Userspace is not able to guess the offset address
>> > of
>> > +	 * kernel kmalloc()ed memory area.
>> > +	 */
>> > +	if (addr)
>> > +		return -EINVAL;
>> > +
>> > +	ptr = io_uring_validate_mmap_request(filp, pgoff, len);
>> > +	if (IS_ERR(ptr))
>> > +		return -ENOMEM;
>> > +
>> > +	info.flags = VM_UNMAPPED_AREA_TOPDOWN;
>> > +	info.length = len;
>> > +	info.low_limit = max(PAGE_SIZE, mmap_min_addr);
>> > +	info.high_limit = arch_get_mmap_base(addr, current->mm->mmap_base);
>> > +#ifdef SHM_COLOUR
>> > +	info.align_mask = PAGE_MASK & (SHM_COLOUR - 1UL);
>> > +#else
>> > +	info.align_mask = PAGE_MASK & (SHMLBA - 1UL);
>> > +#endif
>> > +	info.align_offset = (unsigned long) ptr;
>> > +
>> > +	/*
>> > +	 * A failed mmap() very likely causes application failure,
>> > +	 * so fall back to the bottom-up function here. This scenario
>> > +	 * can happen with large stack limits and large mmap()
>> > +	 * allocations.
>> > +	 */
>> > +	addr = vm_unmapped_area(&info);
>> > +	if (offset_in_page(addr)) {
>> > +		info.flags = 0;
>> > +		info.low_limit = TASK_UNMAPPED_BASE;
>> > +		info.high_limit = mmap_end;
>> > +		addr = vm_unmapped_area(&info);
>> > +	}
>> > +
>> > +	return addr;
>> > +}
>> > +
>> >  #else /* !CONFIG_MMU */
>> >
>> >  static int io_uring_mmap(struct file *file, struct vm_area_struct *vma)
>> > @@ -3529,6 +3578,8 @@ static const struct file_operations io_uring_fops
>> > = {
>> >  #ifndef CONFIG_MMU
>> >  	.get_unmapped_area = io_uring_nommu_get_unmapped_area,
>> >  	.mmap_capabilities = io_uring_nommu_mmap_capabilities,
>> > +#else
>> > +	.get_unmapped_area = io_uring_mmu_get_unmapped_area,
>> >  #endif
>> >  	.poll		= io_uring_poll,
>> >  #ifdef CONFIG_PROC_FS
>> 
>> Hi Jens, Helge - I've bisected a regression with io_uring on ia64 to 
>> this
>> patch in 6.4.  Unfortunately this breaks userspace programs using 
>> io_uring,
>> the easiest one to test is cmake with an io_uring enabled libuv (i.e., 
>> libuv
>> >= 1.45.0) which will hang.
>> 
>> I am aware that ia64 is in a vulnerable place right now which I why I 
>> am
>> keeping this spread limited.  Since this clearly involves
>> architecture-specific changes for parisc,
> 
> it isn't so much architecture-specific... (just one ifdef)
> 
>> is there any chance of looking at
>> what is required to do the same for ia64?  I looked at
>> 0ef36bd2b37815719e31a72d2beecc28ca8ecd26 ("parisc: change value of 
>> SHMLBA
>> from 0x00400000 to PAGE_SIZE") and tried to replicate the SHMLBA ->
>> SHM_COLOUR change, but it made no difference.
>> 
>> If hardware is necessary for testing, I can provide it, including 
>> remote BMC
>> access for restarts/kernel debugging.  Any takers?
> 
> I won't have time to test myself, but maybe you could test?
> 
> Basically we should try to find out why 
> io_uring_mmu_get_unmapped_area()
> doesn't return valid addresses, while arch_get_unmapped_area()
> [in arch/ia64/kernel/sys_ia64.c] does.
> 
> You could apply this patch first:
> It introduces a memory leak (as it requests memory twice), but maybe we
> get an idea?
> The ia64 arch_get_unmapped_area() searches for memory from bottom
> (flags=0), while io_uring function tries top-down first. Maybe that's
> the problem. And I don't understand the offset_in_page() check right
> now.
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 3bca7a79efda..93b1964d2bbb 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -3431,13 +3431,17 @@ static unsigned long 
> io_uring_mmu_get_unmapped_area(struct file *filp,
>  	 * can happen with large stack limits and large mmap()
>  	 * allocations.
>  	 */
> +/* compare to arch_get_unmapped_area() in arch/ia64/kernel/sys_ia64.c 
> */
>  	addr = vm_unmapped_area(&info);
> -	if (offset_in_page(addr)) {
> +printk("io_uring_mmu_get_unmapped_area() address 1 is: %px\n", addr);
> +	addr = NULL;
> +	if (!addr) {
>  		info.flags = 0;
>  		info.low_limit = TASK_UNMAPPED_BASE;
>  		info.high_limit = mmap_end;
>  		addr = vm_unmapped_area(&info);
>  	}
> +printk("io_uring_mmu_get_unmapped_area() returns address %px\n", 
> addr);
> 
>  	return addr;
>  }
> 
> 
> Another option is to disable the call to 
> io_uring_nommu_get_unmapped_area())
> with the next patch. Maybe you could add printks() to ia64's 
> arch_get_unmapped_area()
> and check what it returns there?
> 
> @@ -3654,6 +3658,8 @@ static const struct file_operations io_uring_fops 
> = {
>  #ifndef CONFIG_MMU
>  	.get_unmapped_area = io_uring_nommu_get_unmapped_area,
>  	.mmap_capabilities = io_uring_nommu_mmap_capabilities,
> +#elif 0    /* IS_ENABLED(CONFIG_IA64) */
> +	.get_unmapped_area = NULL,
>  #else
>  	.get_unmapped_area = io_uring_mmu_get_unmapped_area,
>  #endif
> 
> Helge

Thanks Helge.  Sample output from that first patch:

[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() address 1 
is: 1ffffffffff40000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() returns 
address 2000000001e40000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() address 1 
is: 1ffffffffff20000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() returns 
address 2000000001f20000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() address 1 
is: 1ffffffffff30000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() returns 
address 2000000001f30000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() address 1 
is: 1ffffffffff90000
[Wed Jul 12 13:09:50 2023] io_uring_mmu_get_unmapped_area() returns 
address 2000000001f90000

This pattern seems to be pretty stable, I tried instead just directly 
returning the result of a call to arch_get_unmapped_area() at the end of 
the function and it seems similar:

[Wed Jul 12 13:27:07 2023] io_uring_mmu_get_unmapped_area() would return 
address 1ffffffffffd0000
[Wed Jul 12 13:27:07 2023] but arch_get_unmapped_area() would return 
address 2000000001f00000
[Wed Jul 12 13:27:07 2023] io_uring_mmu_get_unmapped_area() would return 
address 1ffffffffff00000
[Wed Jul 12 13:27:07 2023] but arch_get_unmapped_area() would return 
address 1ffffffffff00000
[Wed Jul 12 13:27:07 2023] io_uring_mmu_get_unmapped_area() would return 
address 1fffffffffe20000
[Wed Jul 12 13:27:07 2023] but arch_get_unmapped_area() would return 
address 2000000002000000
[Wed Jul 12 13:27:07 2023] io_uring_mmu_get_unmapped_area() would return 
address 1fffffffffe30000
[Wed Jul 12 13:27:07 2023] but arch_get_unmapped_area() would return 
address 2000000002100000

Is that enough of a clue to go on?