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

From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Cc: "Greg KH" <gregkh@linuxfoundation.org>,
	"amd-gfx list" <amd-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"Alex Deucher" <Alexander.Deucher@amd.com>,
	"Christian König" <christian.koenig@amd.com>,
	"Qiang Yu" <yuq825@gmail.com>
Subject: Re: [PATCH v4 11/14] drm/amdgpu: Guard against write accesses after device removal
Date: Mon, 8 Feb 2021 10:37:19 +0100	[thread overview]
Message-ID: <fcb2cf17-d011-55c6-1545-9fa190e358c3@gmail.com> (raw)
In-Reply-To: <CAKMK7uEJDfPsbnkVfunjVe2iNbpVBWY2_XHai4JntcxWkuVc3A@mail.gmail.com>

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://www.spinics.net/lists/linux-pci/msg103396.html (that was the only reply
>> i got)
> mmiotrace can, but only for debug, and only on x86 platforms:
>
> https://www.kernel.org/doc/html/latest/trace/mmiotrace.html
>
> 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.

>>> 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.

Regards,
Christian.

> -Daniel
>
>> Andrey
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel