From patchwork Wed Jan 10 21:58:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Gustavo A. R. Silva" X-Patchwork-Id: 10157525 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 23A84601A1 for ; Thu, 11 Jan 2018 08:30:22 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0FFD02870F for ; Thu, 11 Jan 2018 08:30:22 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 042932872A; Thu, 11 Jan 2018 08:30:22 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RCVD_NUMERIC_HELO autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5A8FF28701 for ; Thu, 11 Jan 2018 08:30:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A76206E3F7; Thu, 11 Jan 2018 08:30:00 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org X-Greylist: delayed 1501 seconds by postgrey-1.35 at gabe; Wed, 10 Jan 2018 22:46:13 UTC Received: from gateway30.websitewelcome.com (gateway30.websitewelcome.com [192.185.180.41]) by gabe.freedesktop.org (Postfix) with ESMTPS id D26C66E345 for ; Wed, 10 Jan 2018 22:46:13 +0000 (UTC) Received: from cm12.websitewelcome.com (cm12.websitewelcome.com [100.42.49.8]) by gateway30.websitewelcome.com (Postfix) with ESMTP id 6F50D6AA3 for ; Wed, 10 Jan 2018 15:58:50 -0600 (CST) Received: from gator4166.hostgator.com ([108.167.133.22]) by cmsmtp with SMTP id ZOOYeV2UwzzFjZOOYeDIlO; Wed, 10 Jan 2018 15:58:50 -0600 Received: from gator4166.hostgator.com ([108.167.133.22]:39483) by gator4166.hostgator.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.89) (envelope-from ) id 1eZOOX-003RKW-Qe; Wed, 10 Jan 2018 15:58:49 -0600 Received: from 187.172.28.212 ([187.172.28.212]) by gator4166.hostgator.com (Horde Framework) with HTTPS; Wed, 10 Jan 2018 15:58:49 -0600 Date: Wed, 10 Jan 2018 15:58:49 -0600 Message-ID: <20180110155849.Horde.DDGbi3ysasL2eHmvZ4k8adb@gator4166.hostgator.com> From: "Gustavo A. R. Silva" To: Felix Kuehling Subject: Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereferences References: <20180110165008.GA10691@embeddedor.com> In-Reply-To: User-Agent: Horde Application Framework 5 MIME-Version: 1.0 Content-Disposition: inline X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator4166.hostgator.com X-AntiAbuse: Original Domain - lists.freedesktop.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - embeddedor.com X-BWhitelist: no X-Source-IP: 108.167.133.22 X-Source-L: Yes X-Exim-ID: 1eZOOX-003RKW-Qe X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: gator4166.hostgator.com [108.167.133.22]:39483 X-Source-Auth: garsilva@embeddedor.com X-Email-Count: 2 X-Source-Cap: Z3V6aWRpbmU7Z3V6aWRpbmU7Z2F0b3I0MTY2Lmhvc3RnYXRvci5jb20= X-Local-Domain: yes X-Mailman-Approved-At: Thu, 11 Jan 2018 08:29:58 +0000 Cc: David Airlie , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, Alex Deucher , Christian =?utf-8?b?S8O2bmln?= X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP Hi Felix, Quoting Felix Kuehling : > Hi Gustavo, > > Thanks for catching that. When returning a fault, I think you also need > to srcu_read_unlock(&kfd_processes_srcu, idx). > > However, instead of returning an error, I think I'd prefer to skip PDDs > that can't be found with continue statements. That way others would > still suspend and resume successfully. Maybe just print a WARN_ON for > PDDs that aren't found, because that's an unexpected situation, > currently. Maybe in the future it could be normal thing if we ever > support GPU hotplug. > I got it. In that case, what do you think about the following patch instead? index a22fb071..4ff5f0f 100644 mutex_unlock(&p->mutex); Thank you for the feedback. --- Gustavo > Regards, >   Felix > > > On 2018-01-10 11:50 AM, Gustavo A. R. Silva wrote: >> In case kfd_get_process_device_data returns null, there are some >> null pointer dereferences in functions kfd_bind_processes_to_device >> and kfd_unbind_processes_from_device. >> >> Fix this by null checking pdd before dereferencing it. >> >> Addresses-Coverity-ID: 1463794 ("Dereference null return value") >> Addresses-Coverity-ID: 1463772 ("Dereference null return value") >> Signed-off-by: Gustavo A. R. Silva >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> index a22fb071..29d51d5 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> @@ -461,6 +461,13 @@ int kfd_bind_processes_to_device(struct kfd_dev *dev) >> hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) { >> mutex_lock(&p->mutex); >> pdd = kfd_get_process_device_data(dev, p); >> + >> + if (!pdd) { >> + pr_err("Process device data doesn't exist\n"); >> + mutex_unlock(&p->mutex); >> + return -EFAULT; >> + } >> + >> if (pdd->bound != PDD_BOUND_SUSPENDED) { >> mutex_unlock(&p->mutex); >> continue; >> @@ -501,6 +508,11 @@ void kfd_unbind_processes_from_device(struct >> kfd_dev *dev) >> mutex_lock(&p->mutex); >> pdd = kfd_get_process_device_data(dev, p); >> >> + if (!pdd) { >> + mutex_unlock(&p->mutex); >> + return; >> + } >> + >> if (pdd->bound == PDD_BOUND) >> pdd->bound = PDD_BOUND_SUSPENDED; >> mutex_unlock(&p->mutex); --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -461,7 +461,8 @@ int kfd_bind_processes_to_device(struct kfd_dev *dev) hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) { mutex_lock(&p->mutex); pdd = kfd_get_process_device_data(dev, p); - if (pdd->bound != PDD_BOUND_SUSPENDED) { + + if (WARN_ON(!pdd) || pdd->bound != PDD_BOUND_SUSPENDED) { mutex_unlock(&p->mutex); continue; } @@ -501,6 +502,11 @@ void kfd_unbind_processes_from_device(struct kfd_dev *dev) mutex_lock(&p->mutex); pdd = kfd_get_process_device_data(dev, p); + if (WARN_ON(!pdd)) { + mutex_unlock(&p->mutex); + continue; + } + if (pdd->bound == PDD_BOUND) pdd->bound = PDD_BOUND_SUSPENDED;