On Fri, Nov 4, 2016 at 1:32 PM, Eric Engestrom
<eric.engestrom@xxxxxxxxxx> wrote:
> On Friday, 2016-11-04 13:12:51 -0400, Rob Clark wrote:
>> On Fri, Nov 4, 2016 at 12:27 PM, Ville Syrjälä
>> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > On Fri, Nov 04, 2016 at 11:32:45AM -0400, Rob Clark wrote:
>> >> Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>> >>
>> >> drm: make drm_get_format_name thread-safe
>> >>
>> >> Signed-off-by: Eric Engestrom <eric@xxxxxxxxxxxx>
>> >> [danvet: Clarify that the returned pointer must be freed with
>> >> kfree().]
>> >> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> >>
>> >> Note: I think we need to be a bit careful about follow-up audits of
>> >> callers of this.. now that you need to kfree the return value I think
>> >> it is fairly easy for new patches to introduce new callers which leak
>> >> the return value. We probably should have left the function as-is and
>> >> introduce a new variant, or something like that.
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>> >> ---
>> >> drivers/gpu/drm/drm_fourcc.c | 5 ++++-
>> >> 1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> >> index cbb8b77..2be9ea8 100644
>> >> --- a/drivers/gpu/drm/drm_fourcc.c
>> >> +++ b/drivers/gpu/drm/drm_fourcc.c
>> >> @@ -87,7 +87,10 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>> >> */
>> >> char *drm_get_format_name(uint32_t format)
>> >> {
>> >> - char *buf = kmalloc(32, GFP_KERNEL);
>> >> + char *buf = kmalloc(32, GFP_ATOMIC);
>> >> +
>> >> + if (!buf)
>> >> + return NULL;
>
> Unrelated bug, thanks for that fix too :)
>
>> >>
>> >> snprintf(buf, 32,
>> >> "%c%c%c%c %s-endian (0x%08x)",
>> >
>> > Why aren't we using kasprintf()?
>
> Because I didn't know it was a thing :(
> Patch using it incoming.
Thanks
>> >
>> > Or we could have just made the caller provide the buffer...
>>
>> I guess kasprintf() would do the job (although still not address the
>> fact that we still always do the sprintf bits, rather than only doing
>> it when debug logs are enabled).. caller provided buffer would be
>> better. And make it more obvious that something needs to be fixed
>> when merging/rebasing patches that started life before this change
>> landed.
>>
>> I still kinda like the idea of making vsprintf know about fourcc's
>> with a new format string, and just making drm_get_format_name() go
>> away.
>>
>> But I don't really have time atm to re-work all the callers of
>> drm_get_format_name(). So I guess unless someone else does, I'd go w/
>> kasprintf() for now.
>
> That sounds cleaner to me indeed, I'll send a patch doing that tonight.
> Any idea for the name? drm_get_format_name_safe(uint32_t, char*)?
> I could also keep the same name and rely on the function signature
> change to make sure any merging of the old function call will break the
> compilation and be noticed that way.
I think if fxn signature changes, we can keep the same name. What I
get nervous about is fxn sig+name stays same but semantics change ;-)
btw, if this isn't what you were already planning, I'd suggest:
char *drm_get_format_name(uint32_t, char *);
so we can keep it inline w/ DRM_DEBUG_xyz() macros. Or maybe something like:
typedef char drm_format_name_buf[MAX_SIZE]
char *drm_get_format_name(uint32_t, drm_format_name_buf);
??
BR,
-R
> Looks like I'm obviously not qualified to choose which is the best
> option, so I'll leave that to you guys :)
>
> Cheers,
> Eric
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
