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