Re: [2.6 PATCH/RFC] Firmware loader - fix races and resource dealloocation problems

[Manuel Estrada Sainz]

On Sun, Dec 21, 2003 at 01:37:39AM -0500, Dmitry Torokhov wrote:
> Hi,
> 
> It seems that implementation of the firmware loader is racy as it relies
> on kobject hotplug handler. Unfortunately that handler runs too early,
> before firmware class attributes controlling the loading process, are
> created. This causes firmware loading fail at least half of the times on
> my laptop.

 As Greg suggested, make your hotplug handler wait for the files to
 appear and you are set.

 Actually "sleep 1" will generally do the trick.

 What can actually be a problem is that hotplug delays event handling
 while booting, and if firmware needing drivers load at boot time they
 usually timeout before the event gets handled, and when hotplug tryies
 to handle the event the files are already gone.

 The only solution I see to the later is making the default timeout much
 bigger and  maybe having hotplug reset it to a shorter value once it
 starts handling events.
 
> Another problem that I see is that the present implementation tries to free
> some of the allocated resources manually instead of relying on driver model.
> Particularly damaging is freeing fw_priv in request_firmware. Although the
> code calls fw_remove_class_device (which in turns calls 
> class_device_unregister) the freeing of class device and all its attributes
> can be delayed as the attribute files may still be held open by the
> userspace handler or any other program. Subsequent access to these files
> could cause trouble.

 There are actually some oopses, which I believe come from incorrect
 refcounting in sysfs, when I get around to looking at it I'll also
 audit "use after free" issues with firmware_priv struct.

> Also, there is no protection from overwriting firmware image once it has
> been committed.

 The hotplug handler runs as root, and if buggy enough it could write a
 broken firmware image anyway, or write to /dev/mem or whatever, I don't
 see a point in this level of paranoia.
 
> I tried to correct all 3 problems in the patch below. It creates a custom
> hotplug handler that is called from request_hardware. I tried to mimic the
> hotplug handler from kobject - it's nice to have DEVPATH pointing to the
> right place - so I exported kobject_get_path_length and kobject_fill_path
> (former get_kobj_path_length and fill_kobj_patch). I think these 2 should
> not be considered "implementation details" and exporting them is OK.
> Write access to firmware device class attributes is protected by a semaphore
> and the code refuses any updates once firmware loading has been committed or
> aborted. Firmware 

 I am currently on Xmas vacation, until January I won't be very
 responsive.

 Have a nice day

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@xxxxxxxxxx>
                         <ranty@xxxxxxxxxxx>
			 <ranty@xxxxxxxxxxxxxxxxxxxxx>
------------------------ <manuel.estrada@xxxxxxxxxxxxx> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/