Re: [DRM] drm_get_connector_name internal static string buffer causes a race

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 24 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Mar 24, 2014 at 12:06:21PM +0000, Dmitry Malkin wrote:
>> 
>> Hello guys,
>> 
>> I've been playing with reloading intel gfx driver (i915) in a cycle,
>> for a while, and at some point I've found a non-deterministic kernel
>> crash with a highly-variable iteration dependency -- 2 to 200 driver
>> reload iterations.
>> 
>> The apparent race is over the shared internal string buffer in
>> drm_get_connector_name().  It is mostly harmless, due to its results
>> being mostly used for log output, but in at least one case --
>> drm_sysfs_connector_add() -- this leads to a more critical error.
>> 
>> Race scenario:
>> 
>> - drm_sysfs_connector_add()
>>    - drm_get_connector_name()
>> vs.
>> - anything that generates log messages involving DRM connectors
>>    - drm_get_connector_name()
>> 
>>  (and many other from drm_crtc.c) shares with caller const char* to
>> internal static char buffer.  If something call it from other thread,
>> while main thread strore and use returned pointer it may overwrite
>> connector name.
>> 
>> Here are we go: registering HDMI connecter (drm_sysfs_connector_add
>> store and use pointer from drm_get_connector_name) and the same time
>> got VGA connector name down through the stack. (the second thread is
>> upowerd who watch continuously sysfs)
>
> Yeah, in my recent kerneldoc series I've noticed this too and added FIXME
> comments. There's a lot more than just those you've pointed out. The
> problem is that fixing these will be an awful lot of work since you need
> to add proper cleanup code (calling kfree()) to all the required places.
>
> So a full subsystem wide code audit for every single use-site of these.

It would be much easier and much much less error prone to add a name
field (either a fixed size or kmalloced buffer) in drm_connector and
drm_encoder structs, and initialize them in connector/encoder
init. AFAICT the name does not change during the objects' lifetime. None
of the use-sites would need to be changed.

The question is whether (number of connectors + number of encoders) * 32
bytes is too high a price to pay for the implementation simplicity. I
think not.

Another alternative to returning kmalloced strings is:

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 16ca28ed5ee8..85c8ed191daa 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -277,11 +277,10 @@ EXPORT_SYMBOL(drm_get_encoder_name);
  *
  * FIXME: This isn't really multithreading safe.
  */
-const char *drm_get_connector_name(const struct drm_connector *connector)
+const char *drm_get_connector_name(char *buf, size_t bufsize,
+				   const struct drm_connector *connector)
 {
-	static char buf[32];
-
-	snprintf(buf, 32, "%s-%d",
+	snprintf(buf, bufsize, "%s-%d",
 		 drm_connector_enum_list[connector->connector_type].name,
 		 connector->connector_type_id);
 	return buf;

So the callers may use the kind of buffers they want. Something like
this may be needed for drm_get_format_name() anyway.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center