public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Helge Deller <[email protected]>,
	[email protected], [email protected],
	John David Anglin <[email protected]>
Subject: Re: [PATCH v2] io_uring: Adjust mapping wrt architecture aliasing requirements
Date: Thu, 16 Feb 2023 11:00:47 -0700	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 2/16/23 10:52?AM, Helge Deller wrote:
> On 2/16/23 17:46, Jens Axboe wrote:
>> On 2/16/23 9:33?AM, Helge Deller wrote:
>>> On 2/16/23 17:11, Jens Axboe wrote:
>>>> On 2/16/23 1:09?AM, Helge Deller wrote:
>>>>> 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]>
>>>>> ---
>>>>> v2: Do not allow to map to a user-provided addresss. This forces
>>>>> programs to write portable code, as usually on x86 mapping to any
>>>>> address will succeed, while it will fail for most provided address if
>>>>> used on stricter architectures.
>>>>>
>>>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>>>> index 862e05e6691d..01fe7437a071 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>
>>>>> @@ -3059,6 +3060,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;
>>>>
>>>> Can we relax this so that if the address is correctly aligned, it will
>>>> allow it?
>>>
>>> My previous patch had it relaxed, but after some more thoughts I removed
>>> it in this v2-version again.
>>>
>>> The idea behind it is good, but I see a huge disadvantage in allowing
>>> correctly aligned addresses: People develop their code usually on x86
>>> which has no such alignment requirements, as it just needs to be PAGE_SIZE aligned.
>>> So their code will always work fine on x86, but as soon as the same code
>>> is built on other platforms it will break. As you know, on parisc it's pure luck
>>> if the program chooses an address which is correctly aligned.
>>> I'm one of the debian maintainers for parisc, and I've seen similiar
>>> mmap-issues in other programs as well. Everytime I've found it to be wrong,
>>> you have to explain to the developers what's wrong and sometimes it's
>>> not easy to fix it.
>>> So, if we can educate people from assuming their code to be correct, I think
>>> we can save a lot of additional work afterwards.
>>> That said, I think it's better to be strict now, unless someone comes
>>> up with a really good reason why it needs to be less strict.
>>
>> I don't disagree with the reasoning at all, but the problem is that it
>> may introduce breakage if someone IS doing the right thing. Is it
>> guaranteed to be true? No, certainly not. But someone could very well be
>> writing perfectly portable code and mapping a ring into a specific
>> address, and this will now break.
> 
> We will find out if there are such users if we keep it strict now and
> open it up if it's really necessary. If you open it up now, you won't
> be able to turn it stricter later.

But it has been open up until now, that's the issue. And you're now
trying to make it stricter, which is indeed later...

>> AFAICT, this is actually the case with the syzbot case. In fact, with
>> the patch applied, it'll obviously start crashing on all archs as the
>> mmaps will now return -EINVAL rather than work.
> 
> Yes, but it's not a real user and just a (invalid) testcase.
> For that I think it's OK to just disable it.

Totally agree, and I did just disable it, but that part of the test is
not invalid. I don't care about this particular test case, it's more of
a general concern.

-- 
Jens Axboe


  reply	other threads:[~2023-02-16 18:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-16  8:09 [PATCH v2] io_uring: Adjust mapping wrt architecture aliasing requirements Helge Deller
2023-02-16 16:11 ` Jens Axboe
2023-02-16 16:33   ` Helge Deller
2023-02-16 16:46     ` Jens Axboe
2023-02-16 17:52       ` Helge Deller
2023-02-16 18:00         ` Jens Axboe [this message]
2023-06-27 14:14 ` Jiri Slaby
2023-06-27 19:24   ` Helge Deller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox