On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko
<dmitry.osipenko at collabora.com> wrote:>
> On 8/11/22 01:03, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
> > <dmitry.osipenko at collabora.com> wrote:
> >>
> >> On 8/10/22 18:08, Rob Clark wrote:
> >>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter <daniel at
ffwll.ch> wrote:
> >>>>
> >>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko
wrote:
> >>>>> On 7/6/22 00:48, Rob Clark wrote:
> >>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian K?nig
<christian.koenig at amd.com> wrote:
> >>>>>>>
> >>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>>>>>>> Drivers that use drm_gem_mmap() and
drm_gem_mmap_obj() helpers don't
> >>>>>>>> handle imported dma-bufs properly, which
results in mapping of something
> >>>>>>>> else than the imported dma-buf. On NVIDIA
Tegra we get a hard lockup when
> >>>>>>>> userspace writes to the memory mapping of
a dma-buf that was imported into
> >>>>>>>> Tegra's DRM GEM.
> >>>>>>>>
> >>>>>>>> Majority of DRM drivers prohibit mapping
of the imported GEM objects.
> >>>>>>>> Mapping of imported GEMs require special
care from userspace since it
> >>>>>>>> should sync dma-buf because mapping
coherency of the exporter device may
> >>>>>>>> not match the DRM device. Let's
prohibit the mapping for all DRM drivers
> >>>>>>>> for consistency.
> >>>>>>>>
> >>>>>>>> Suggested-by: Thomas Hellstr?m
<thomas.hellstrom at linux.intel.com>
> >>>>>>>> Signed-off-by: Dmitry Osipenko
<dmitry.osipenko at collabora.com>
> >>>>>>>
> >>>>>>> I'm pretty sure that this is the right
approach, but it's certainly more
> >>>>>>> than possible that somebody abused this
already.
> >>>>>>
> >>>>>> I suspect that this is abused if you run deqp cts
on android.. ie. all
> >>>>>> winsys buffers are dma-buf imports from gralloc.
And then when you
> >>>>>> hit readpix...
> >>>>>>
> >>>>>> You might only hit this in scenarios with separate
gpu and display (or
> >>>>>> dGPU+iGPU) because self-imports are handled
differently in
> >>>>>> drm_gem_prime_import_dev().. and maybe not in
cases where you end up
> >>>>>> with a blit from tiled/compressed to linear..
maybe that narrows the
> >>>>>> scope enough to just fix it in userspace?
> >>>>>
> >>>>> Given that that only drivers which use DRM-SHMEM
potentially could've
> >>>>> map imported dma-bufs (Panfrost, Lima) and they
already don't allow to
> >>>>> do that, I think we're good.
> >>>>
> >>>> So can I have an ack from Rob here or are there still
questions that this
> >>>> might go boom?
> >>>>
> >>>> Dmitry, since you have a bunch of patches merged now I
think would also be
> >>>> good to get commit rights so you can drive this more
yourself. I've asked
> >>>> Daniel Stone to help you out with getting that.
> >>>
> >>> I *think* we'd be ok with this on msm, mostly just by dumb
luck.
> >>> Because the dma-buf's we import will be self-import.
I'm less sure
> >>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to
have a
> >>> special path for imported dma-bufs either, and in that case
they won't
> >>> be self-imports.. but I guess no one has tried to run android
cts on
> >>> panfrost).
> >>
> >> The last time I tried to mmap dma-buf imported to Panfrost
didn't work
> >> because Panfrost didn't implement something needed for that.
I'll need
> >> to take a look again because can't recall what it was.
> >>
> >>> What about something less drastic to start, like (apologies
for
> >>> hand-edited patch):
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c
b/drivers/gpu/drm/drm_gem.c
> >>> index 86d670c71286..fc9ec42fa0ab 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct
drm_gem_object
> >>> *obj, unsigned long obj_size,
> >>> {
> >>> int ret;
> >>>
> >>> + WARN_ON_ONCE(obj->import_attach);
> >>
> >> This will hang NVIDIA Tegra, which is what this patch fixed
initially.
> >> If neither of upstream DRM drivers need to map imported dma-bufs
and
> >> never needed, then why do we need this?
> >
> > oh, tegra isn't using shmem helpers? I assumed it was. Well my
point
> > was to make a more targeted fail on tegra, and a WARN_ON for everyone
> > else to make it clear that what they are doing is undefined behavior.
> > Because so far existing userspace (or well, panfrost and freedreno at
> > least, those are the two I know or checked) don't make special
cases
> > for mmap'ing against the dmabuf fd against the dmabuf fd instead
of
> > the drm device fd.
>
> It's not clear to me what bad Android does form yours comments. Does it
> export dma-buf from GPU and then import it to GPU? If yes, then DRM core
> has a check for the self-importing [1].
>
> [1]
>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L918
>
> If you're meaning something else, then please explain in a more
details.
So, android/gralloc allocates buffers externally to the driver and
imports them into driver. (And that seems to not just be window
surfaces, but in cases random textures, etc)
In the normal case these should be allocated from drm/msm so it should
hit [1].. this is the "dumb luck" I mentioned earlier. But I'm
not
confident enough to say that there is no other case.
>
> > I *think* it should work out that we don't hit this path with
> > freedreno but on android I can't really guarantee or prove it. So
> > your patch would potentially break existing working userspace. Maybe
> > it is userspace that isn't portable (but OTOH it isn't like
you are
> > going to be using freedreno on tegra). So why don't you go for a
more
> > targeted fix that only returns an error on hw where this is
> > problematic?
>
> That's what the first versions of the patch did and Christian suggested
> that it's not a good approach. In fact it should be not only Tegra that
> has a broken dma-buf mapping, but apparently OMAP driver too.
Hmm, I guess I'm a bit more conservative when it comes to potentially
breaking userspace.
BR,
-R
>
> --
> Best regards,
> Dmitry