linux-kernel - Re: [PATCH] drm: move allocation out of drm_get_format_name()

Open Source and information security mailing list archives

Message-ID: <CAF6AEGt5Mz1S5h+pth2bz820ZirsXcEt0X7k5Z4P0q180ASg3A@mail.gmail.com>
Date:   Sun, 6 Nov 2016 08:03:47 -0500
From:   Rob Clark <robdclark@...il.com>
To:     Christian König <christian.koenig@....com>
Cc:     Eric Engestrom <eric@...estrom.ch>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Wei Yongjun <yongjun_wei@...ndmicro.com.cn>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Flora Cui <Flora.Cui@....com>,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
        Tom St Denis <tom.stdenis@....com>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        VMware Graphics <linux-graphics-maintainer@...are.com>,
        Vitaly Prosyak <vitaly.prosyak@....com>,
        Alexandre Demers <alexandre.f.demers@...il.com>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        Emily Deng <Emily.Deng@....com>,
        Ken Wang <Qingqing.Wang@....com>,
        Junwei Zhang <Jerry.Zhang@....com>,
        Michel Dänzer <michel.daenzer@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alex Deucher <alexander.deucher@....com>,
        Colin Ian King <colin.king@...onical.com>
Subject: Re: [PATCH] drm: move allocation out of drm_get_format_name()

On Sun, Nov 6, 2016 at 4:47 AM, Christian König
<christian.koenig@....com> wrote:
> Am 05.11.2016 um 17:49 schrieb Rob Clark:
>>
>> On Sat, Nov 5, 2016 at 12:38 PM, Eric Engestrom <eric@...estrom.ch> wrote:
>>>
>>> On Saturday, 2016-11-05 13:11:36 +0100, Christian König wrote:
>>>>
>>>> Am 05.11.2016 um 02:33 schrieb Eric Engestrom:
>>>>>
>>>>> +typedef char drm_format_name_buf[32];
>>>>
>>>> Please don't use a typedef for this, just define the maximum size of
>>>> characters the function might write somewhere.
>>>>
>>>> See the kernel coding style as well:
>>>>>
>>>>> In general, a pointer, or a struct that has elements that can
>>>>> reasonably
>>>>> be directly accessed should **never** be a typedef.
>>>
>>> I would normally agree as I tend to hate typedefs ($DAYJOB {ab,mis}uses
>>> them way too much), and your way was what I wrote at first, but Rob
>>> Clark's
>>> typedef idea makes it much harder for someone to allocate a buffer of
>>> the wrong size, which IMO is good thing here.
>>
>> IMHO I would make a small test program to verify this actually helps
>> the compiler catch problems.  And if it does, I would stick with it.
>> The coding-style should be guidelines, not something that supersedes
>> common sense / practicality.
>
>
> Well completely agree that we should be able to question the coding style
> rules, but when we do it we discuss this on a the mailing list first and
> then start to use it in code. Not the other way around.

if I'm not mistaken, that is what we are doing ;-)

>>
>> That is my $0.02 anyways.. if others vehemently disagree and want to
>> dogmatically stick to the coding-style guidelines, ok then.  OTOH, if
>> this approach doesn't help the compiler catch issues, then it isn't
>> worth it.
>
>
> Yeah, exactly that's the point. If I'm not completely mistaken the compiler
> won't issue a warning here if you pass an array with the wrong size.
>
> I think you need something like "struct drm_format_name_buf { char str[32];
> };" to trigger this.

hmm, actually the struct is a nice idea then if the compiler wouldn't
catch the wrong-size-array

> Apart from that is this function really called so often that using
> kasprintf() is a problem here? Or is there another motivation behind the
> change?

Two things trouble me about the kasprintf approach.. (ignoring the
fact that atm it is not GFP_ATOMIC)
1) you can't do DRM_DEBUG("format: %s\n", drm_get_format_name(..)) so
it pulls the memory allocation and sprintf outside of the drm_debug
check
2) seems awfully easy to forget the kfree...  I wouldn't have even
known that now you need to free the result (with some patches I'm
working on) if it weren't for the fact that lockdep alerted me to the
GFP_KERNEL allocation in atomic ctx ;-)

BR,
-R

> Regards,
> Christian.
>
>
>>
>> BR,
>> -R
>>
>>> I can rewrite the typedef out if you think it's better.
>>>
>>> Cheers,
>>>    Eric
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@...ts.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives