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

Open Source and information security mailing list archives

Message-ID: <20161108101558.ihvrprbbdqjwu5wg@phenom.ffwll.local>
Date:   Tue, 8 Nov 2016 11:15:58 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Jani Nikula <jani.nikula@...ux.intel.com>
Cc:     Eric Engestrom <eric.engestrom@...tec.com>,
        David Airlie <airlied@...ux.ie>,
        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>,
        Chunming Zhou <David1.Zhou@....com>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Sinclair Yeh <syeh@...are.com>,
        Xinliang Liu <z.liuxinliang@...ilicon.com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        VMware Graphics <linux-graphics-maintainer@...are.com>,
        Vitaly Prosyak <vitaly.prosyak@....com>,
        Alexandre Demers <alexandre.f.demers@...il.com>,
        intel-gfx@...ts.freedesktop.org,
        Eric Engestrom <eric@...estrom.ch>,
        Emily Deng <Emily.Deng@....com>,
        Junwei Zhang <Jerry.Zhang@....com>,
        Michel Dänzer <michel.daenzer@....com>,
        linux-kernel@...r.kernel.org,
        Alex Deucher <alexander.deucher@....com>,
        Colin Ian King <colin.king@...onical.com>,
        Christian König <christian.koenig@....com>
Subject: Re: [Intel-gfx] [PATCH v2] drm: move allocation out of
 drm_get_format_name()

On Mon, Nov 07, 2016 at 07:38:25PM +0200, Jani Nikula wrote:
> 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. ;)

Can you pls resend with that and corrected Fixes and all the acks
collected?
-Daniel

> 
> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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