Re: Linux 4.2-rc1

[Linus Torvalds]

On Wed, Jul 8, 2015 at 5:58 PM, Ming Lei <ming.lei@xxxxxxxxxxxxx> wrote:
> On Thu, Jul 9, 2015 at 1:29 AM, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>> Also, it looks like you need to hold the "fw_lock" to even look at
>> that pointer, since the buffer can get reallocated etc.
>
> Yes, the above code with holding 'fw_lock' is right fix for the issue since
> sysfs read can happen anytime, and there is one race between firmware
> request abort and reading uevent of sysfs.

So if fw_priv->buf is NULL, what should we do?

Should we skip the TIMEOUT= and ASYNC= fields too?

Something like the attached, perhaps?

Shuah, how reproducible is this? Does this (completely untested) patch
make any difference?

                  Linus
 drivers/base/firmware_class.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 9c4288362a8e..894bda114224 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -563,10 +563,8 @@ static void fw_dev_release(struct device *dev)
 	kfree(fw_priv);
 }
 
-static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
+static int do_firmware_uevent(struct firmware_priv *fw_priv, struct kobj_uevent_env *env)
 {
-	struct firmware_priv *fw_priv = to_firmware_priv(dev);
-
 	if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->buf->fw_id))
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
@@ -577,6 +575,18 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	int err = 0;
+
+	mutex_lock(&fw_lock);
+	if (fw_priv->buf)
+		err = do_firmware_uevent(fw_priv, env);
+	mutex_unlock(&fw_lock);
+	return err;
+}
+
 static struct class firmware_class = {
 	.name		= "firmware",
 	.class_attrs	= firmware_class_attrs,