| Message ID | 1573155554-16248-1-git-send-email-cohens@codeaurora.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | allow DRM drivers to limit creation of blobs | expand |
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? Also, why are your fuzzers not also allocating enormous amounts of gem buffers, which will also exhaust memory eventually? -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
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. > 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. > -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
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