Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Mime-Version: 1.0
Subject: RE: [PATCH v2] dma-buf/heaps: system_heap: Avoid DoS by limiting
 single allocations to half of all memory
Reply-To: jaewon31.kim@samsung.com
Sender: Jaewon Kim <jaewon31.kim@samsung.com>
From:   Jaewon Kim <jaewon31.kim@samsung.com>
To:     "T.J. Mercier" <tjmercier@google.com>,
        Andrew Morton <akpm@linux-foundation.org>
CC:     John Stultz <jstultz@google.com>,
        Jaewon Kim <jaewon31.kim@samsung.com>,
        "sumit.semwal@linaro.org" <sumit.semwal@linaro.org>,
        "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
        "hannes@cmpxchg.org" <hannes@cmpxchg.org>,
        "mhocko@kernel.org" <mhocko@kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jaewon31.kim@gmail.com" <jaewon31.kim@gmail.com>
In-Reply-To: <CABdmKX3NzTWHa3K_rna1iY+UERUrYK1Rq9WqUt9VQRaAKZsWwg@mail.gmail.com>
Message-ID: <20230407022419epcms1p424f1f8a7100aeccac62651ce25cd6140@epcms1p4>
Date:   Fri, 07 Apr 2023 11:24:19 +0900
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CABdmKX3NzTWHa3K_rna1iY+UERUrYK1Rq9WqUt9VQRaAKZsWwg@mail.gmail.com>
        <20230405185650.239f9721f066aa480e83d543@linux-foundation.org>
        <20230405172524.e25b62e1c548a95564b1d324@linux-foundation.org>
        <20230406000854.25764-1-jaewon31.kim@samsung.com>
        <20230406014419epcms1p3f285b6e3fdbb1457db1bcbaab9e863be@epcms1p3>
        <20230406021712epcms1p216f274040d25d18380668ffbfa809c48@epcms1p2>
        <20230405200923.9b0dca2165ef3335a0f6b112@linux-foundation.org>
        <CANDhNCq+mhhXjW7huYoQwUq18sRCcn6HjbPqh7mU9KrGyKeLfA@mail.gmail.com>
        <CABdmKX0ZnQX5BPEJjqxwUsx=c8ykcX8Bjitak50DAYvfti6VFw@mail.gmail.com>
        <20230406163822.faae6a90b3aa4942df6e7442@linux-foundation.org>
        <CGME20230406000841epcas1p3630010a770682be0f1d540a448f3e00e@epcms1p4>
Precedence: bulk

>On Thu, Apr 6, 2023 at 4:38?PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>> On Thu, 6 Apr 2023 16:27:28 -0700 "T.J. Mercier" <tjmercier@google.com> wrote:
>>
>> > > When you say "decide what's the largest reasonable size", I think it
>> > > is difficult as with the variety of RAM sizes and buffer sizes I don't
>> > > think there's a fixed limit. Systems with more ram will use larger
>> > > buffers for image/video capture buffers.  And yes, you're right that
>> > > ram/2-1 in a single allocation is just as broken, but I'm not sure how
>> > > to establish a better guard rail.
>> > >
>> > > thanks
>> > > -john
>> >
>> > I like ENOMEM with the len / PAGE_SIZE > totalram_pages() check and
>> > WARN_ON. We know for sure that's an invalid request, and it's pretty
>> > cheap to check as opposed to trying a bunch of reclaim before failing.
>>
>> Well, if some buggy caller has gone and requested eleventy bigabytes of
>:)
>> memory, doing a lot of reclaiming before failing isn't really a problem
>> - we don't want to optimize for this case!
>>
>The issue I see is that it could delay other non-buggy callers, or
>cause reclaim that wouldn't have happened if we just outright rejected
>a known-bad allocation request from the beginning.
>
>> > For buffers smaller than that I agree with John in that I'm not sure
>> > there's a definitive threshold.
>>
>> Well...  why do we want to do _anything_ here?  Why cater for buggy
>> callers?  I think it's because "dma-buf behaves really badly with very
>> large allocation requests".  Again, can we fix that instead?
>>
>There are a variety of different allocation strategies used by
>different exporters so I don't think there's one dma-buf thing we
>could fix for slow, large allocations in general. For the system_heap
>in this patch it's really just alloc_pages. I'm saying I don't think
>the kernel should ever ask alloc_pages for more memory than exists on
>the system, which seems like a pretty reasonable sanity check to me.
>Given that, I don't think we should do anything for buffers smaller
>than totalram_pages() (except maybe to prevent OOM panics via
>__GFP_RETRY_MAYFAIL when we attempt to exhaust system memory on any
>request - valid or otherwise).

I think T. J. also agree with me on what I shared.
  if (len / PAGE_SIZE > totalram_pages()) return ERR_PTR(-ENOMEM);
  #define LOW_ORDER_GFP (GFP_HIGHUSER | __GFP_ZERO | __GFP_COMP | __GFP_RETRY_MAYFAIL)

Regarding the dma-buf behavior, I also would like to say that the dma-buf
system heap seems to be designed to allocate that large memory. In mobile
devices, we need that large memory for camera buffers or grahpics
rendendering buffers. So that large memory should be allowed but the invalid
huge size over ram should be avoided.

I agree on that mm should reclaim even for the large size. But that reclaim
process may affect system performance or user convenience. In that perspective
I thought ram / 2 was reasonable, but yes not a golden value. I hope to use
just ram size as sanity check.

Additionally if all agree, we may be able to apply __GFP_RETRY_MAYFAIL too.

BR