Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 9 Jan 2020 11:49:17 +0100
From: Thomas Zimmermann <>
To: Christian König <>,
 Alex Deucher <>, Kees Cook <>
Cc:, David Airlie <>,
 Greg Kroah-Hartman <>,
 LKML <>,
 amd-gfx list <>,
 Tianlin Li <>,
 Maling list - DRI developers <>,
 Alex Deucher <>
Subject: Re: [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check
 the return value


Am 09.01.20 um 11:15 schrieb Christian König:
> Am 08.01.20 um 18:51 schrieb Alex Deucher:
>> On Wed, Jan 8, 2020 at 12:39 PM Kees Cook <> wrote:
>>> On Wed, Jan 08, 2020 at 01:56:47PM +0100, Christian König wrote:
>>>> Am 07.01.20 um 20:25 schrieb Tianlin Li:
>>>>> Right now several architectures allow their set_memory_*() family of
>>>>> functions to fail, but callers may not be checking the return values.
>>>>> If set_memory_*() returns with an error, call-site assumptions may be
>>>>> infact wrong to assume that it would either succeed or not succeed at
>>>>> all. Ideally, the failure of set_memory_*() should be passed up the
>>>>> call stack, and callers should examine the failure and deal with it.
>>>>> Need to fix the callers and add the __must_check attribute. They also
>>>>> may not provide any level of atomicity, in the sense that the memory
>>>>> protections may be left incomplete on failure. This issue likely has a
>>>>> few steps on effects architectures:
>>>>> 1)Have all callers of set_memory_*() helpers check the return value.
>>>>> 2)Add __must_check to all set_memory_*() helpers so that new uses do
>>>>> not ignore the return value.
>>>>> 3)Add atomicity to the calls so that the memory protections aren't
>>>>> left
>>>>> in a partial state.
>>>>> This series is part of step 1. Make drm/radeon check the return
>>>>> value of
>>>>> set_memory_*().
>>>> I'm a little hesitate merge that. This hardware is >15 years old and
>>>> nobody
>>>> of the developers have any system left to test this change on.
>>> If that's true it should be removed from the tree. We need to be able to
>>> correctly make these kinds of changes in the kernel.
>> This driver supports about 15 years of hardware generations.  Newer
>> cards are still prevalent, but the older stuff is less so.  It still
>> works and people use it based on feedback I've seen, but the older
>> stuff has no active development at this point.  This change just
>> happens to target those older chips.
> Just a few weeks back we've got a mail from somebody using an integrated
> R128 in a laptop.
> After a few mails back and force we figured out that his nearly 20 years
> old hardware was finally failing.
> Up till that he was still successfully updating his kernel from time to
> time and the driver still worked. I find that pretty impressive.
>> Alex
>>>> Would it be to much of a problem to just add something like: r =
>>>> set_memory_*(); (void)r; /* Intentionally ignored */.
>>> This seems like a bad idea -- we shouldn't be papering over failures
>>> like this when there is logic available to deal with it.
> Well I certainly agree to that, but we are talking about a call which
> happens only once during driver load/unload. If necessary we could also
> print an error when something goes wrong, but please no larger
> refactoring of return values and call paths.

IMHO radeon should be marked as orphaned or obsolete then.

Best regards

> It is perfectly possible that this call actually failed on somebodies
> hardware, but we never noticed because the driver still works fine. If
> we now handle the error it is possible that the module never loads and
> the user gets a black screen instead.
> Regards,
> Christian.
>>>> Apart from that certainly a good idea to add __must_check to the
>>>> functions.
>>> Agreed!
>>> -Kees
>>> -- 
>>> Kees Cook
>>> _______________________________________________
>>> dri-devel mailing list
> _______________________________________________
> dri-devel mailing list

Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.