On Mon, 24 Mar 2014, Daniel Vetter <daniel@xxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
