Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Steve Cohen <cohens@codeaurora.org>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Alistair Strachan <adelva@google.com>,
	pdhaval@codeaurora.org, Sean Paul <seanpaul@chromium.org>
Subject: Re: [PATCH 0/3] allow DRM drivers to limit creation of blobs
Date: Sun, 10 Nov 2019 21:57:48 +0100
Message-ID: <CAKMK7uEi=sBsmumX5Fxm1jcM665yL3P6tWVhS_p9zCic02N3HA@mail.gmail.com> (raw)
In-Reply-To: <7072bcc51eb44d49ab4266e0ec216df6@codeaurora.org>

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

      reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-07 19:39 Steve Cohen
2019-11-07 19:39 ` [PATCH 1/3] drm: add driver hook for create blob limitations Steve Cohen
2019-11-07 19:39 ` [PATCH 2/3] drm/msm: add support for createblob_check driver hook Steve Cohen
2019-11-07 19:39 ` [PATCH 3/3] drm/msm/dpu: check blob limitations during create blob ioctl Steve Cohen
2019-11-08  8:34 ` [PATCH 0/3] allow DRM drivers to limit creation of blobs Daniel Vetter
2019-11-09  0:01   ` cohens
2019-11-10 20:57     ` Daniel Vetter [this message]

Reply instructions:

You may reply publically 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 \
    --in-reply-to='CAKMK7uEi=sBsmumX5Fxm1jcM665yL3P6tWVhS_p9zCic02N3HA@mail.gmail.com' \
    --to=daniel@ffwll.ch \
    --cc=adelva@google.com \
    --cc=cohens@codeaurora.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=pdhaval@codeaurora.org \
    --cc=seanpaul@chromium.org \
    /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

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git