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: <20161107171244.GK25290@imgtec.com>
Date:   Mon, 7 Nov 2016 17:12:44 +0000
From:   Eric Engestrom <eric.engestrom@...tec.com>
To:     Jani Nikula <jani.nikula@...ux.intel.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 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?

> > @@ -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