[PATCH v4 11/14] drm/amdgpu: Guard against write accesses after device removal

[Christian König]

Am 08.02.21 um 23:15 schrieb Andrey Grodzovsky:
>
> On 2/8/21 11:23 AM, Daniel Vetter wrote:
>> On Mon, Feb 8, 2021 at 3:00 PM Christian König 
>> <christian.koenig at amd.com> wrote:
>>> Am 08.02.21 um 11:11 schrieb Daniel Vetter:
>>>> On Mon, Feb 08, 2021 at 11:03:15AM +0100, Christian König wrote:
>>>>> Am 08.02.21 um 10:48 schrieb Daniel Vetter:
>>>>>> On Mon, Feb 08, 2021 at 10:37:19AM +0100, Christian König wrote:
>>>>>>> Am 07.02.21 um 22:50 schrieb Daniel Vetter:
>>>>>>>> [SNIP]
>>>>>>>>> Clarification - as far as I know there are no page fault 
>>>>>>>>> handlers for kernel
>>>>>>>>> mappings. And we are talking about kernel mappings here, right 
>>>>>>>>> ?  If there were
>>>>>>>>> I could solve all those issues the same as I do for user 
>>>>>>>>> mappings, by
>>>>>>>>> invalidating all existing mappings in the kernel (both kmaps 
>>>>>>>>> and ioreamps)and
>>>>>>>>> insert dummy zero or ~0 filled page instead.
>>>>>>>>> Also, I assume forcefully remapping the IO BAR to ~0 filled 
>>>>>>>>> page would involve
>>>>>>>>> ioremap API and it's not something that I think can be easily 
>>>>>>>>> done according to
>>>>>>>>> am answer i got to a related topic a few weeks ago
>>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-pci%2Fmsg103396.html&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C9d1bdf4cee504cd71b4908d8cc4df310%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637483982454608249%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Anw%2BOwJ%2B5tvjW3tmkVNdz13%2BZ18vdpfOLWqsUZL7D2I%3D&reserved=0 
>>>>>>>>> (that was the only reply
>>>>>>>>> i got)
>>>>>>>> mmiotrace can, but only for debug, and only on x86 platforms:
>>>>>>>>
>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Flatest%2Ftrace%2Fmmiotrace.html&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C9d1bdf4cee504cd71b4908d8cc4df310%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637483982454608249%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Wa7BFNySVQJLyD6WY4pZHTP1QfeZwD7F5ydBrXuxppQ%3D&reserved=0 
>>>>>>>>
>>>>>>>>
>>>>>>>> Should be feasible (but maybe not worth the effort) to extend 
>>>>>>>> this to
>>>>>>>> support fake unplug.
>>>>>>> Mhm, interesting idea you guys brought up here.
>>>>>>>
>>>>>>> We don't need a page fault for this to work, all we need to do 
>>>>>>> is to insert
>>>>>>> dummy PTEs into the kernels page table at the place where 
>>>>>>> previously the
>>>>>>> MMIO mapping has been.
>>>>>> Simply pte trick isn't enough, because we need:
>>>>>> - drop all writes silently
>>>>>> - all reads return 0xff
>>>>>>
>>>>>> ptes can't do that themselves, we minimally need write protection 
>>>>>> and then
>>>>>> silently proceed on each write fault without restarting the 
>>>>>> instruction.
>>>>>> Better would be to only catch reads, but x86 doesn't do 
>>>>>> write-only pte
>>>>>> permissions afaik.
>>>>> You are not thinking far enough :)
>>>>>
>>>>> The dummy PTE is point to a dummy MMIO page which is just never used.
>>>>>
>>>>> That hast the exact same properties than our removed MMIO space 
>>>>> just doesn't
>>>>> goes bananas when a new device is MMIO mapped into that and our 
>>>>> driver still
>>>>> tries to write there.
>>>> Hm, but where do we get such a "guaranteed never used" mmio page from?
>>> Well we have tons of unused IO space on 64bit systems these days.
>>>
>>> Doesn't really needs to be PCIe address space, doesn't it?
>> That sounds very trusting to modern systems not decoding random
>> ranges. E.g. the pci code stopped extending the host bridge windows on
>> its own, entirely relying on the acpi provided ranges, to avoid
>> stomping on stuff that's the but not listed anywhere.
>>
>> I guess if we have a range behind a pci bridge, which isn't used by
>> any device, but decoded by the bridge, then that should be safe
>> enough. Maybe could even have an option in upstream to do that on
>> unplug, if a certain flag is set, or a cmdline option.
>> -Daniel
>
>
> Question - Why can't we just set those PTEs to point to system memory 
> (another RO dummy page)
> filled with 1s ?


Then writes are not discarded. E.g. the 1s would change to something else.

Christian.


>
> Andrey
>
>
>>
>>> Christian.
>>>
>>>> It's a nifty idea indeed otherwise ...
>>>> -Daniel
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>
>>>>>>>>>> But ugh ...
>>>>>>>>>>
>>>>>>>>>> Otoh validating an entire driver like amdgpu without such a 
>>>>>>>>>> trick
>>>>>>>>>> against 0xff reads is practically impossible. So maybe you 
>>>>>>>>>> need to add
>>>>>>>>>> this as one of the tasks here?
>>>>>>>>> Or I could just for validation purposes return ~0 from all reg 
>>>>>>>>> reads in the code
>>>>>>>>> and ignore writes if drm_dev_unplugged, this could already 
>>>>>>>>> easily validate a big
>>>>>>>>> portion of the code flow under such scenario.
>>>>>>>> Hm yeah if your really wrap them all, that should work too. Since
>>>>>>>> iommappings have __iomem pointer type, as long as amdgpu is sparse
>>>>>>>> warning free, should be doable to guarantee this.
>>>>>>> Problem is that ~0 is not always a valid register value.
>>>>>>>
>>>>>>> You would need to audit every register read that it doesn't use 
>>>>>>> the returned
>>>>>>> value blindly as index or similar. That is quite a bit of work.
>>>>>> Yeah that's the entire crux here :-/
>>>>>> -Daniel
>>