linux-kernel - Re: [PATCH v2] drm: move allocation out of drm_get_format

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87eg2n8a26.fsf@intel.com>
Date:   Mon, 07 Nov 2016 19:38:25 +0200
From:   Jani Nikula <jani.nikula@...ux.intel.com>
To:     Eric Engestrom <eric.engestrom@...tec.com>
Cc:     Eric Engestrom <eric@...estrom.ch>, linux-kernel@...r.kernel.org,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Rob Clark <robdclark@...il.com>,
        Christian König <christian.koenig@....com>,
        Alex Deucher <alexander.deucher@....com>,
        David Airlie <airlied@...ux.ie>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        VMware Graphics <linux-graphics-maintainer@...are.com>,
        Sinclair Yeh <syeh@...are.com>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Tom St Denis <tom.stdenis@....com>,
        Michel Dänzer <michel.daenzer@....com>,
        Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
        Emily Deng <Emily.Deng@....com>,
        Chunming Zhou <David1.Zhou@....com>,
        Flora Cui <Flora.Cui@....com>,
        Vitaly Prosyak <vitaly.prosyak@....com>,
        Colin Ian King <colin.king@...onical.com>,
        Ken Wang <Qingqing.Wang@....com>,
        Alexandre Demers <alexandre.f.demers@...il.com>,
        Junwei Zhang <Jerry.Zhang@....com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        Wei Yongjun <yongjun_wei@...ndmicro.com.cn>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org
Subject: Re: [PATCH v2] drm: move allocation out of drm_get_format_name()

On Mon, 07 Nov 2016, Eric Engestrom <eric.engestrom@...tec.com> wrote:
> On Monday, 2016-11-07 10:10:13 +0200, Jani Nikula wrote:
>> On Mon, 07 Nov 2016, Eric Engestrom <eric@...estrom.ch> wrote:
>> > Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>> >
>> >     drm: make drm_get_format_name thread-safe
>> >
>> >     Signed-off-by: Eric Engestrom <eric@...estrom.ch>
>> >     [danvet: Clarify that the returned pointer must be freed with
>> >     kfree().]
>> >     Signed-off-by: Daniel Vetter <daniel.vetter@...ll.ch>
>> 
>> The Fixes: format is:
>> 
>> Fixes: 90844f00049e ("drm: make drm_get_format_name thread-safe")
>> 
>> But is this a fix, really, or just an improvement? What exactly is the
>> bug being fixed? The commit message is not sufficient.
>
> "The function's behaviour was changed in 90844f00049e, without changing
> its signature, causing people to keep using it the old way without
> realising they were now leaking memory.
> Rob Clark also noticed it was also allocating GFP_KERNEL memory in
> atomic contexts, breaking them.
>
> Instead of having to allocate GFP_ATOMIC memory and fixing the callers
> to make them cleanup the memory afterwards, let's change the function's
> signature by having the caller take care of the memory and passing it to
> the function.
> The new parameter is a single-field struct in order to enforce the size
> of its buffer and help callers to correctly manage their memory."
>
> Does this sound good?

It's fine; no need to go overboard. ;)

BR,
Jani.

>
>> > @@ -54,6 +62,6 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>> >  int drm_format_vert_chroma_subsampling(uint32_t format);
>> >  int drm_format_plane_width(int width, uint32_t format, int plane);
>> >  int drm_format_plane_height(int height, uint32_t format, int plane);
>> > -char *drm_get_format_name(uint32_t format) __malloc;
>> > +char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>> 
>> I wonder if it would be better to make that return "const char *". If
>> the user really wants to look under the hood, there's buf->str. *shrug*
>
> Good idea, I'll do that in v3 with the proper commit msg and tags. It'll
> have to wait another day though, -ENOTIME and all.
>
>> 
>> With the commit message improved,
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@...el.com>
>
> Cheers :)
>   Eric

-- 
Jani Nikula, Intel Open Source Technology Center