[PATCH] drm: make drm_get_format_name atomic/irq safe again

[Rob Clark]

On Fri, Nov 4, 2016 at 9:23 PM, Eric Engestrom <eric at engestrom.ch> wrote:
> On Friday, 2016-11-04 13:50:25 -0400, Rob Clark wrote:
>> On Fri, Nov 4, 2016 at 1:32 PM, Eric Engestrom
>> <eric.engestrom at imgtec.com> 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 at linux.intel.com> 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 at engestrom.ch>
>> >> >>     [danvet: Clarify that the returned pointer must be freed with
>> >> >>     kfree().]
>> >> >>     Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> >> >>
>> >> >> 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 at gmail.com>
>> >> >> ---
>> >> >>  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.
>
> This was exactly my idea.
>
>> Or maybe something like:
>>
>>    typedef char drm_format_name_buf[MAX_SIZE]
>>    char *drm_get_format_name(uint32_t, drm_format_name_buf);
>>
>> ??
>
> I didn't think of that, but that would avoid mistakes about the size of
> the buffer. Modifying the code now, I'll send the patch in a few minutes.

I guess it would be worth a double check if gcc could catch the error
if someone passes in a too-small buffer, but assuming it does, I think
this approach would be simpler/safer than the "make it look like
snprintf()" approach.. but if compiler doesn't catch it, maybe it is
better to go w/ Ville's approach.

/me hoping at least a modern gcc is clever enough..

BR,
-R


> Cheers,
>   Eric