Subject: Re: Oops in 3.10.99 -- NULL pointer dereference in radeon_fence_ref
To: Luis Henriques <luis.henriques@canonical.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <20160307025014.GA9499@mail.codepoet.org>
 <20160307204654.GB6545@kroah.com> <56DDED67.2030801@amd.com>
 <20160307225851.GB25867@kroah.com> <20160309135612.GA20283@charon.olymp>
CC: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        <andersen@codepoet.org>, linux-kernel <linux-kernel@vger.kernel.org>,
        <stable@vger.kernel.org>, Sasha Levin <sasha.levin@oracle.com>,
        Jiri Slaby <jslaby@suse.cz>, Kamal Mostafa <kamal@canonical.com>
From: =?UTF-8?Q?Nicolai_H=c3=a4hnle?= <nicolai.haehnle@amd.com>
Message-ID: <56E04FFA.7070906@amd.com>
Date: Wed, 9 Mar 2016 11:31:54 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160309135612.GA20283@charon.olymp>
Content-Type: multipart/mixed;
	boundary="------------090506060305070802030409"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2016 16:32:00.6106
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0750
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 6261
Lines: 148

--------------090506060305070802030409
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 8bit

On 09.03.2016 08:56, Luis Henriques wrote:
> On Mon, Mar 07, 2016 at 02:58:51PM -0800, Greg Kroah-Hartman wrote:
>> On Mon, Mar 07, 2016 at 10:06:47PM +0100, Christian K?nig wrote:
>>> Am 07.03.2016 um 21:46 schrieb Greg Kroah-Hartman:
>>>> On Sun, Mar 06, 2016 at 07:50:14PM -0700, Erik Andersen wrote:
>>>>> The following patch to radeon_sa_bo_new that
>>>>> went into 3.10.99
>>>>>
>>>>>    commit 8d5e1e5af0c667545c202e8f4051f77aa3bf31b7
>>>>>    Author: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>>>>    Date:   Fri Feb 5 14:35:53 2016 -0500
>>>>>      drm/radeon: hold reference to fences in radeon_sa_bo_new
>>>>>      commit f6ff4f67cdf8455d0a4226eeeaf5af17c37d05eb upstream.
>>>>>
>>>>> is triggering an Oops for me right when xscreensaver
>>>>> first began doing 3D stuff.  After reverting this
>>>>> patch, xscreensaver has been happily running 3D stuff.
>>>>>
>>>>> Mar  6 18:00:43 sage kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>>>>> Mar  6 18:00:43 sage kernel: IP: [<ffffffffa010345d>] radeon_fence_ref+0xd/0x50 [radeon]
>>>>> Mar  6 18:00:43 sage kernel: PGD 799e1d067 PUD 819186067 PMD 0
>>>>> Mar  6 18:00:43 sage kernel: Oops: 0002 [#1] SMP
>>>>>
>>>>> Mar  6 18:00:43 sage kernel: Stack:
>>>>> Mar  6 18:00:43 sage kernel:  ffffffffa01607ec ffff88108a4e8000 ffff88108a4e8000 ffff880888fbc000
>>>>> Mar  6 18:00:43 sage kernel:  ffff880ecbf11c78 0000fe2001000006 0000000000000000 0020000000000100
>>>>> Mar  6 18:00:43 sage kernel:  00000000000d1200 ffff880ecbf11c14 0000000000000000 0000000000000000
>>>>> Mar  6 18:00:43 sage kernel: Call Trace:
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffffa01607ec>] ? radeon_sa_bo_new+0x2ac/0x4f0 [radeon]
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffffa005fc9d>] ? ttm_eu_list_ref_sub+0x3d/0x60 [ttm]
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffffa0117c49>] radeon_ib_get+0x39/0x110 [radeon]
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffffa011a4ea>] radeon_cs_ioctl+0x69a/0xa70 [radeon]
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffffa008e2d2>] drm_ioctl+0x512/0x650 [drm]
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffff810a46e1>] ? do_futex+0x111/0xc30
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffff81182a45>] do_vfs_ioctl+0x305/0x520
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffff8107cd39>] ? vtime_account_user+0x69/0x80
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffff81182ce1>] SyS_ioctl+0x81/0xa0
>>>>> Mar  6 18:00:43 sage kernel:  [<ffffffff8178210f>] tracesys+0xe1/0xe6
>>>>>
>>>>> $ lspci | grep VGA
>>>>> 03:00.0 VGA compatible controller: Advanced Micro Devices, Inc.
>>>>> [AMD/ATI] Redwood XT [Radeon HD 5670/5690/5730]
>>>> Next time, please cc: the people responsible for that patch as well...
>>>>
>>>> I can revert it, but maybe something else is going on here?  Do you have
>>>> this same problem on 3.14, and 4.5-rc7?
>>>
>>> Hi Greg,
>>>
>>> yes that's an already known issue. Feel free to revert that one for now.
>>>
>>> I got it on my TODO list to provide a fixed patch for older kernel, but that
>>> can take a while.
>>>
>>> For the background Nicolais patch is correct, but assumes that
>>> radeon_fence_unref() can safely take NULL as the fence which is not the case
>>> for older kernels.

Actually, the call to radeon_fence_ref() is the culprit.

>>
>> Ok, thanks, now reverted.
>>
>
> And looks like a few more kernels may be affected as well.  I'll
> revert it from 3.16 kernel, and I'm adding Kamal, Sasha and Jiri to
> the CC list.

Kernels that contain commit 954605ca "drm/radeon: use common fence 
implementation for fences, v4" are safe, older kernels require a 
NULL-pointer check around the call to radeon_fence_ref.

This means kernels 3.17 and older are affected and need the additional 
NULL pointer check that I've sent out already on a different thread (I'm 
attaching it again, hoping that Erik gets a chance to test it).

It would be nice to get a confirmation that this really does fix the 
observed bug, then I can prepare a fixed version of the patch for 3.17 
and older (i.e. squash the original bad commit with the attached patch).

Cheers,
Nicolai

>
> Cheers,
> --
> Lu?s
>
>> greg k-h
>> --
>> To unsubscribe from this list: send the line "unsubscribe stable" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--------------090506060305070802030409
Content-Type: text/x-patch;
	name="0001-drm-radeon-guard-call-to-radeon_fence_ref-against-NU.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment;
	filename*0="0001-drm-radeon-guard-call-to-radeon_fence_ref-against-NU.pa";
	filename*1="tch"

>From 85d028178d9772f2a07e4ed156820d95c4e0ad18 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle@amd.com>
Date: Mon, 7 Mar 2016 23:41:52 -0300
Subject: [PATCH] drm/radeon: guard call to radeon_fence_ref against NULL
 pointers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Candidate fix for a kernel oops that was introduced by the backport of
commit 954605ca3 "drm/radeon: hold reference to fences in radeon_sa_bo_new"
to kernels where radeon does not use the common fence implementation for
fences.

Reported-by: Lutz Euler <lutz.euler@freenet.de>
Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
---
 drivers/gpu/drm/radeon/radeon_sa.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_sa.c b/drivers/gpu/drm/radeon/radeon_sa.c
index 197b157..7d11901 100644
--- a/drivers/gpu/drm/radeon/radeon_sa.c
+++ b/drivers/gpu/drm/radeon/radeon_sa.c
@@ -349,8 +349,10 @@ int radeon_sa_bo_new(struct radeon_device *rdev,
 			/* see if we can skip over some allocations */
 		} while (radeon_sa_bo_next_hole(sa_manager, fences, tries));
 
-		for (i = 0; i < RADEON_NUM_RINGS; ++i)
-			radeon_fence_ref(fences[i]);
+		for (i = 0; i < RADEON_NUM_RINGS; ++i) {
+			if (fences[i])
+				radeon_fence_ref(fences[i]);
+		}
 
 		spin_unlock(&sa_manager->wq.lock);
 		r = radeon_fence_wait_any(rdev, fences, false);
-- 
2.5.0


--------------090506060305070802030409--