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.