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;
Message-ID: <ad11c2f2-abe9-5483-addd-15b0c1328fab@amd.com>
Date:   Wed, 19 Oct 2022 15:24:20 -0400
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [PATCH] drm/amdkfd: Fix memory leak in kfd_mem_dmamap_userptr()
To:     Rafael Mendonca <rafaelmendsr@gmail.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Ramesh Errabolu <Ramesh.Errabolu@amd.com>,
        Oak Zeng <Oak.Zeng@amd.com>
Cc:     amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20221018012751.887460-1-rafaelmendsr@gmail.com>
Content-Language: en-US
From:   Felix Kuehling <felix.kuehling@amd.com>
In-Reply-To: <20221018012751.887460-1-rafaelmendsr@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NHNXU3ZacVp3dVhnVjYyaUZJcSt4MTJrNXRmVDlZcTFTbFhBOVEwbzVOaFhB?=
 =?utf-8?B?a2VhTDN0YXl6cHB2T3R3azFqK011NXhnZndQSmx3eFJ0Nk1VRmVSNWlDQy9I?=
 =?utf-8?B?KzkxTFkrMlRqdk5uMHNNTnBQbjExakVKVGVxeGVvRmZEWWlrM3NpdHhsbXV4?=
 =?utf-8?B?Vk1jbnM1bHV3NnhMc2RySkNwUTIvWFEwUUFEUkJkWHVRdDU1M3RnOHJxaWFR?=
 =?utf-8?B?d3ljSUp0M2l0emlrMUR0a0Q0d2JNWWxQV0VIZ2FIa29tODdMWlhKZ0drUUli?=
 =?utf-8?B?NStKTHVieVJhTlJjdjVNQ0M5NC9FdklaRjB1cTg4MWxpeVhhK1VIRFhTRUlz?=
 =?utf-8?B?Q2Fvekx2YUMycHBVb3lVSHNnVnVKQ3BTcEtpM05HS1RmWElLZTdCMkxnSGpQ?=
 =?utf-8?B?YnhTUWcrRDU1NDhDY01wNjBUNVUyTThaeHJiZWpLelEvQ2xkQTFXSEIyY25z?=
 =?utf-8?B?UUJ2QmV5VFJ2MkVOZ2lzbEdCUUg5R1pabGxkdktsbGJUUFAxb0Q2MGIxcW85?=
 =?utf-8?B?aU1zQkRGY2FrTUJvczJiTi9yaUFuOWN1dWNIdzYzazFadUc2azQ1ekYwRkVY?=
 =?utf-8?B?Q0IrQnN3TjE3cjkrTEl3SXhwNHM3WEp1dHNya0g1OWgwRldBdDlCVlc3dXlB?=
 =?utf-8?B?VXlGRFByYllzMGh3NXlpYzJ0R21LZ2VNMUdqQjRTK3crQTZ6TXZyeC9kUTd6?=
 =?utf-8?B?K0NHRFNpRXNuSDFLN2lPaEJsNXF3b2xSNEg1NlZSRHpyMEdLbUpEa3RtU3NE?=
 =?utf-8?B?NU56V0ZOc3p0NmorVmhvZlFPa1BLdkZmdVNWeTlVNmRXL2ovK2dVTVRTZzBN?=
 =?utf-8?B?OGJVQ3U5c0p2RjIzSkhSWTIyZHFUV1BBVkUvcldOdVV2R3NocDdmOC9KTE5Q?=
 =?utf-8?B?NEdWdEllSjA2U2xQQ0lhUVQ2K0YwMWt3VDBpeGM5NFJQWHYrZFB3UTAxR2po?=
 =?utf-8?B?eWphcHd2ckxid3FDczNRckpTM0dTRkg3MGtLL2dtRkRXMmcyMGVFNG9ERWVL?=
 =?utf-8?B?RGV3Q3FEUUI5ZW9IbzU0aGhpU21sZ0pwOUt3SndlUzI1K09Za3pzSUF1REZC?=
 =?utf-8?B?cnBkWk1vUWxuNi92TWk3M1A4bm1VYlcyVlVYU2EzNlZNTkhoYkh2S3RuVG1O?=
 =?utf-8?B?eVoyMDF5WXdRd2FJK0k0cG9QSG8zZzcvTU96K0dFVW9QbW1zY01ZL3JKSkJ1?=
 =?utf-8?B?OWd2Vzc1S29YNkY5WDFJSHYyUDdIT3c1Tjk5bi96RFliYzVSR3FXalhrdVVP?=
 =?utf-8?B?N2gyVW1BS2N6aTFISGlJc2h4L1Q5ak51UlIrS1Q5R1hQalc1QjZiNVFSb3hn?=
 =?utf-8?B?czdJT21aSGFsU2NaYXdnNmpRSzE4dGZ0VzdIOWhaa3plaEpZMmJOOGFHTDNK?=
 =?utf-8?B?bGZWK3o2L3VlTjlnblhQUzd5Rlp3djhqZFRxOVVveXlWN25aVlpxaG1ZUlpW?=
 =?utf-8?B?dTdLM0ZBeWM0Znd6aHFubEZIc3FQQmJ5bWYwUHpubnM2K2xBRlFweVpNMitN?=
 =?utf-8?B?Q1NadGVkdDI2dWpsb0FVaXd5RndNdkpyVlNUNWNIOUJ5aklNamdkbjRObW9W?=
 =?utf-8?B?ZFdBeHhyVGN3V003YXlJT0JjR1FDbzJROGp6Qnh1MzF3Qy9LdG9iM3NZZzc5?=
 =?utf-8?B?Mk1BOFBxSnBOZHVQUk9ScGEvcmhKUjVYSllJZWxIUVBlZGZka2JJM1BmV2ds?=
 =?utf-8?B?UVdTVE4vL1ZjVEQ3YWwvWGRZTmx2YTB1ZXhnTEtaM0pSQm03ZitUVjllSlAx?=
 =?utf-8?B?cDNlcTFNYVg3bkE1bjkwV3hnaWVJdHpicXhZeHJ5bmE0ZHBkVnFUVnFSL2Yz?=
 =?utf-8?B?clRSYWFoWVFsRHVWY3dWdzQ5dDhJajlhRGJkbzI1MmgwUiswQUl1SXFUeFhW?=
 =?utf-8?B?cnZTN2Zzb3hLOU1SeHpsbW5oRXhEQjRvL013a2VDd2w3bDkwNHd2Z01BSko3?=
 =?utf-8?B?MGtJUHNuQVhiSXB4RW4za2ZtdVgvWGpsTWh4ZU4rZ1c4clRzQkxaaUdSSTlt?=
 =?utf-8?B?U2dNU1RIRDRBYmg1REJENG44Nit5QkRrUEpTNGtHWDFKdHpYSExnYTBRU3FF?=
 =?utf-8?B?VUhFSEszNUNjUUk3Um9DaFFvdUV0TzUxZXVDeWREZ1FoQWVkV2Z5eUlNOHJD?=
 =?utf-8?Q?d0eRgLJnit9yG+PeZp+NRuhb4?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e1acd7ef-3320-41b0-7bde-08dab2078807
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Oct 2022 19:24:24.4615
 (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: ZgaEH0gZn9VQiIGnOscGHV3AM7quCKdUmLVZpJ2zIU+u5BD7eMXLTtBp2371ZL9uoUwtkAw/VNjR1VKpOs5Ohw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5722
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2022-10-17 21:27, Rafael Mendonca wrote:
> If the number of pages from the userptr BO differs from the SG BO then the
> allocated memory for the SG table doesn't get freed before returning
> -EINVAL, which may lead to a memory leak in some error paths. Fix this by
> checking the number of pages before allocating memory for the SG table.
>
> Fixes: 264fb4d332f5 ("drm/amdgpu: Add multi-GPU DMA mapping helpers")
> Signed-off-by: Rafael Mendonca <rafaelmendsr@gmail.com>

Thank you for catching this. The patch is

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

I submitted it to our amd-staging-drm-next branch.

Regards,
   Felix


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 978d3970b5cc..84f44f7e4111 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -510,13 +510,13 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem,
>   	struct ttm_tt *ttm = bo->tbo.ttm;
>   	int ret;
>   
> +	if (WARN_ON(ttm->num_pages != src_ttm->num_pages))
> +		return -EINVAL;
> +
>   	ttm->sg = kmalloc(sizeof(*ttm->sg), GFP_KERNEL);
>   	if (unlikely(!ttm->sg))
>   		return -ENOMEM;
>   
> -	if (WARN_ON(ttm->num_pages != src_ttm->num_pages))
> -		return -EINVAL;
> -
>   	/* Same sequence as in amdgpu_ttm_tt_pin_userptr */
>   	ret = sg_alloc_table_from_pages(ttm->sg, src_ttm->pages,
>   					ttm->num_pages, 0,