Re: [PATCH 0/3] allow DRM drivers to limit creation of blobs

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Nov 9, 2019 at 1:01 AM <cohens@codeaurora.org> wrote:
>
> On 2019-11-08 03:34, Daniel Vetter wrote:
> > On Thu, Nov 07, 2019 at 02:39:11PM -0500, Steve Cohen wrote:
> >> Fuzzers used in Android compliance testing repeatedly call the
> >> create blob IOCTL which eventually exhausts the system memory.
> >> This series adds a hook which allows drivers to impose their own
> >> limitations on the size and/or number of blobs created.
> >
> > Pretty sure this isn't just a problem for msm/dpu alone, why this very
> > limited approach?
> >
> I'm not familiar enough with the blob requirements for other
> vendor's drivers to impose any restrictions on them. The idea
> was to provide the hook for vendors to implement their own
> checks. Support for msm/mdp* drivers will be added in v2 if this
> approach is acceptable.

Yeah, but I don't think different limits (and then maybe different
tunables for these different limits) on drivers isn't a great idea.
Plus I thought this kind of stuff was supposed to be catched by the
kernel memory cgroup controller. Does that not work? The blob stuff is
maybe a bit too direct way to allocate kernel memory, but there's many
others I've thought. This all just feels a lot like busywork to check
an android compliance checkbox, without really digging into the
underlying problem and fixing it for real.

One approach that would work I think would be:
- Extended the blob property type to have min/max size (we might need
a range for some), set it for all current blob properties. To do that
we'd need to create a new property creation function for blobs, which
takes those limits. There's not a hole lot of them, so should be
doable.
- In the create blob function we walk all property (or book-keep that
somewhere) and check the blob isn't too big.
- We also validate the size when setting the property, at least some
of that code from various places.

I think this would actually improve the situation here, instead of
whack-a-mole. The cheaper cope-out would be if we simply limit the
total property size to something big enough, but not unlimited, like
16MB or something like that.

> > Also, why are your fuzzers not also allocating enormous amounts of gem
> > buffers, which will also exhaust memory eventually?
>
> Excellent question... This will likely come in a follow-up series.

Would be good to know that, so we can solve this for real, not just a
one-off for the compliance checkbox. Also really wondering why cgroups
doesn't catch this.
-Daniel

>
> > -Daniel
> >
> >>
> >> Steve Cohen (3):
> >>   drm: add driver hook for create blob limitations
> >>   drm/msm: add support for createblob_check driver hook
> >>   drm/msm/dpu: check blob limitations during create blob ioctl
> >>
> >>  drivers/gpu/drm/drm_property.c          |  7 +++++++
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++++++++++++++
> >>  drivers/gpu/drm/msm/msm_drv.c           | 25
> >> +++++++++++++++++++++++++
> >>  drivers/gpu/drm/msm/msm_kms.h           |  1 +
> >>  include/drm/drm_drv.h                   |  9 +++++++++
> >>  5 files changed, 56 insertions(+)
> >>
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum,
> >> a Linux Foundation Collaborative Project
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch