Subject: Re: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation
To:     John Stultz <john.stultz@linaro.org>
Cc:     "Ruhl, Michael J" <michael.j.ruhl@intel.com>,
        "guangming.cao@mediatek.com" <guangming.cao@mediatek.com>,
        "sumit.semwal@linaro.org" <sumit.semwal@linaro.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "wsd_upstream@mediatek.com" <wsd_upstream@mediatek.com>,
        "libo.kang@mediatek.com" <libo.kang@mediatek.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "yf.wang@mediatek.com" <yf.wang@mediatek.com>,
        "linaro-mm-sig@lists.linaro.org" <linaro-mm-sig@lists.linaro.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        "lmark@codeaurora.org" <lmark@codeaurora.org>,
        "benjamin.gaignard@linaro.org" <benjamin.gaignard@linaro.org>,
        "bo.song@mediatek.com" <bo.song@mediatek.com>,
        "matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
        "labbott@redhat.com" <labbott@redhat.com>,
        "mingyuan.ma@mediatek.com" <mingyuan.ma@mediatek.com>,
        "jianjiao.zeng@mediatek.com" <jianjiao.zeng@mediatek.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
References: <CAO_48GF=ttKqSOm9GRoA3Mq+-RQOtRjWp449XPcz-wH=kjaTjw@mail.gmail.com>
 <20220113123406.11520-1-guangming.cao@mediatek.com>
 <4f88205c1b344aea8608960e2f85b8f4@intel.com>
 <e657f5257cbf4955817b0bbf037de9f9@intel.com>
 <24157767-dc29-bbdd-5428-d89ecc6b9606@amd.com>
 <CALAqxLXRtYDNQ8y1efVGa4SwUH_oAaHviZFjsOVSNFmUHnCCeQ@mail.gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <6b8182a1-7cdc-7369-5c34-e6d0c24efcca@amd.com>
Date:   Fri, 14 Jan 2022 08:16:30 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <CALAqxLXRtYDNQ8y1efVGa4SwUH_oAaHviZFjsOVSNFmUHnCCeQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cDNnVUwva1Y2MmdzeUdNRXowa3dTYWVoR0kyM0ZKQy80eEJIbXFhR3hUeTQ3?=
 =?utf-8?B?ckRzKzdFMUZCZ3BZNEJWQ0V2WUYyblFabTc3a3JZNTJVSXBPRkgzQU5mNHJX?=
 =?utf-8?B?NEl5azg0MmpuampjVWZTZmVGd01iNUVxeUxkdm5DRFdUa1RBVjBZMmRLMS94?=
 =?utf-8?B?ZlJMY0hQTTk3YzNsbityU3pXajZMV2RDY2hKSFY0aVB6UEhGQ1FIZ1lFeW42?=
 =?utf-8?B?aklZcTVzemNGbEpqamhtbUhqT05hMm5NSTBXdGFzb2tsMzNUTVNRMEhTMmVq?=
 =?utf-8?B?OUU2QWp2RlVycGFzOWUyNEhPZ2RJZGJhZE9FYVF5QkcwTWpxZzAyeVQ1Uk85?=
 =?utf-8?B?MDNNNFRSa3lvU3QvbjVnWnpJNHBVZUFBazFQMHpwN2kwTzQvOTdEQjZrZWtU?=
 =?utf-8?B?ZUcrdWplZ3lTalNVMkVrSHFpN2hqdEpnSDVYU3JhVWYyRXJDQzdYempTQjBu?=
 =?utf-8?B?U0V2TFZyMjlQVEpEM3A1WUx5VkpnOE50ekREcHRlenVuS0ExWE00cHlTK2p1?=
 =?utf-8?B?QS8yWnltSWRibnBrM1JaMG1PSzNtaUFUYVQrcVorMnM4ZmIvNC9GSTl3VlJh?=
 =?utf-8?B?eHR0MzYzcGxPeUowOTlubEdTdVZnM3ZOUVJya3ZkeGd3ZlNOUmNIaWJyVUhs?=
 =?utf-8?B?Ky9tU2NZZEd2ZlFqOGJCSXZhZkt1aExYN0RLdnhDMUZ0bHhQRDNSRnREczVs?=
 =?utf-8?B?YU0xbFFsQUp1c2tOck9vV3M2WWVCbE85YkhGRUhSTjdRUnJXanR2RW1DazRS?=
 =?utf-8?B?QkhpdlFlclNZSzBqMndkNm5NWHdjMUJLN3NOZTY0dTBSY0RsN2RhM2lzM1lS?=
 =?utf-8?B?bTQ1VU9lYWxKODdqS01mdDZzNWwrUlRJcERaWjlDcmEwckcvZERla0lMRnhP?=
 =?utf-8?B?K0J0VjNMeHVEUjA1RFFqSmgxQ1JzSmp1MDBVczNMb25CbzE3WVdjT1EwS0V0?=
 =?utf-8?B?QVlEQURXRE5aUndzQ3VjV0dhMm40NGpKOFFJeFZwNTNFTU45YzU0WEcyZmRX?=
 =?utf-8?B?MEZ0SjlpaW11dEVPMjZGK3pYT0R0cmh2RU5hZTBJemwwckQwUXd2ZGU2NE1p?=
 =?utf-8?B?VzdMZlNhS0wvYjhLWVJqSHhqYjNPMi9wSkt5Ui91R1NmT3hJYU80bTFIbHFK?=
 =?utf-8?B?MzMwdzNYNXRKTUJKdCtMRkNjMjQ5NDBrMVpxZmc3K1JveWxzTlpkZi9XMUFW?=
 =?utf-8?B?UXlFVmdweldEeGFUeTlYNkZhWllVNkxiZ3drQ2tPVkE2c2ZFZFVrZmtZMGZL?=
 =?utf-8?B?Mm5wNU5iVjhxVlFmSUdQNkpSZ3VWMnJuVnlTNUJxeEJQOFRBeWg4dzlBOGJQ?=
 =?utf-8?B?MUg0bGI5VEY1VkZCUWlkL2tpVEs3ZDBVcmtKZWtObllNaGJyU296Y3Z3VmI1?=
 =?utf-8?B?Q08vcXQvUFBwNzAveEJTcUhGWGNkRWEwNE9LT1g5YXVEZ0FzRDdrblpVUWNS?=
 =?utf-8?B?dXFOOXhRNDFYVkpFSnpwODFiTytBb05jK3VxRmpqYm5lUVVBcm5CVXNJUHJZ?=
 =?utf-8?B?Rk1BZ1NPRVB5UW5uK1Rtck4zLzVPTTJZQmhWbHdhblZzZjVwaFpjYS9USVVQ?=
 =?utf-8?B?UHViVVl2RnVSQzlFSzd6c2pLakxzNWp3dnFCUlFPdlViUFpkeGxpNDFJdmdR?=
 =?utf-8?B?OC80bmhtNGhUbTNKYy9ZZnlhRGxhVWh0KzZBUFBpaTJVOVh3Rk5ZNG9LZ3Iz?=
 =?utf-8?B?OUp3em1wN2w5Mmh6VENyVWJadUgrUTJRanVLZDBzYWpDeGVvYWRwcHZIL1Zx?=
 =?utf-8?B?bDcxVGlrZmxmRG1wSGNMSlVJaUdWbGtCaXB5RzNaQS8veDlkQUVSQVJWL2xK?=
 =?utf-8?B?NjVUOWpiWnR2R0gyNmM1NHVWRWQ5eHN6WmZaeTZ4ZTJiblY4bmY2OG1pVHov?=
 =?utf-8?B?M1BOWmc0ZnNCeC9RVG5VSEtvMU5RdjRvajNtbC8rQy9oRHlPU2ZDc1ZMZG8r?=
 =?utf-8?B?aXlRNjBhZ1l2L1NhTjQ2cDFTYkU1UUlJY0g5ZG9JUXFrKzJCbnpCVWU2SEgx?=
 =?utf-8?B?WGxWcndncm5hUDRwR1dZcnM5U3U4LytnVFlwVVRsTzJ2YWFkWU1aUkZnM01H?=
 =?utf-8?B?dU05eHZyR1NFUk9icU1NRXBsSEN6aU1wbnNMZnRyQkRDV3RuZHd2VTl3MWZi?=
 =?utf-8?Q?BoLI=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 523cd04d-5346-49f8-7d54-08d9d72dcf9a
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1201MB0192.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jan 2022 07:16:40.8175
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RPy5XcfbxMexvhvTV6UrXBjMOLo17XYewv0gQDBGswg7pUNAEydXCY+xMyebvhiM
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1201MB0210
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 14.01.22 um 00:26 schrieb John Stultz:
> On Thu, Jan 13, 2022 at 5:05 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 13.01.22 um 14:00 schrieb Ruhl, Michael J:
>>>> -----Original Message-----
>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>>> Ruhl, Michael J
>>>>> -----Original Message-----
>>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>>>> guangming.cao@mediatek.com
>>>>> +   /*
>>>>> +    * Invalid size check. The "len" should be less than totalram.
>>>>> +    *
>>>>> +    * Without this check, once the invalid size allocation runs on a process
>>>>> that
>>>>> +    * can't be killed by OOM flow(such as "gralloc" on Android devices), it
>>>>> will
>>>>> +    * cause a kernel exception, and to make matters worse, we can't find
>>>>> who are using
>>>>> +    * so many memory with "dma_buf_debug_show" since the relevant
>>>>> dma-buf hasn't exported.
>>>>> +    */
>>>>> +   if (len >> PAGE_SHIFT > totalram_pages())
>>>> If your "heap" is from cma, is this still a valid check?
>>> And thinking a bit further, if I create a heap from something else (say device memory),
>>> you will need to be able to figure out the maximum allowable check for the specific
>>> heap.
>>>
>>> Maybe the heap needs a callback for max size?
>> Well we currently maintain a separate allocator and don't use dma-heap,
>> but yes we have systems with 16GiB device and only 8GiB system memory so
>> that check here is certainly not correct.
> Good point.
>
>> In general I would rather let the system run into -ENOMEM or -EINVAL
>> from the allocator instead.
> Probably the simpler solution is to push the allocation check to the
> heap driver, rather than doing it at the top level here.
>
> For CMA or other contiguous heaps, letting the allocator fail is fast
> enough. For noncontiguous buffers, like the system heap, the
> allocation can burn a lot of time and consume a lot of memory (causing
> other trouble) before a large allocation might naturally fail.

Yeah, letting a alloc_page() loop run for a while is usually not nice at 
all :)

You can still do a sanity check here, e.g. the size should never have 
the most significant bit set for example.

Regards,
Christian.

>
> thanks
> -john