linux-kernel - Re: [RESEND][RFC][PATCH] leds: fix oops race in led trigger registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1219306014.5385.9.camel@dax.rpnet.com>
Date:	Thu, 21 Aug 2008 09:06:54 +0100
From:	Richard Purdie <rpurdie@...ys.net>
To:	Vegard Nossum <vegard.nossum@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Zdenek Kabelac <zdenek.kabelac@...il.com>,
	Henrique de Moraes Holschuh <hmh@....eng.br>,
	linux-kernel@...r.kernel.org
Subject: Re: [RESEND][RFC][PATCH] leds: fix oops race in led trigger
	registration

Hi,

On Thu, 2008-08-21 at 09:17 +0200, Vegard Nossum wrote:
> I resubmit this, which was posted a month ago, with no responses from
> the maintainer (and no recent fixes in this area either). I have not
> tested it myself, but I am reasonably confident that this is an
> obviously correct fix. I guess it would not hurt to exercise it as well.
> 
> Zdenek: Have recent kernels also been showing this behaviour? Did the
> patch fix the problem for you? You said something about a new issue
> showing up, do you think that was related to this patch?
> 
> Also, just one question (since I'm newbie at this): Is it dangerous to
> register the &dev_attr_trigger device file before we put the device on
> the list or initialize it completely? If so, the initialization should
> probably be moved all to the top before the file is made public.

This patch isn't quite right since whilst it fixes one race, it opens up
a number of other potential problems. We definitely should have the led
listed in leds_list before calling the various functions and the
existing init order is correct in that sense.

I suspect your problem is caused by something calling a trigger
registration between the point its added to the list and the
init_rwsem(). The best solution is to move the init_rwsem() to earlier
in the function. Yes, that means some ifdef ugliness but so be it. Could
you see whether you still see the problem with the patch below?

Cheers,

Richard


diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 559a408..c9d8219 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -113,6 +113,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	if (rc)
 		goto err_out;
 
+#ifdef CONFIG_LEDS_TRIGGERS
+	init_rwsem(&led_cdev->trigger_lock);
+#endif
 	/* add to the list of leds */
 	down_write(&leds_list_lock);
 	list_add_tail(&led_cdev->node, &leds_list);
@@ -121,8 +124,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	led_update_brightness(led_cdev);
 
 #ifdef CONFIG_LEDS_TRIGGERS
-	init_rwsem(&led_cdev->trigger_lock);
-
 	rc = device_create_file(led_cdev->dev, &dev_attr_trigger);
 	if (rc)
 		goto err_out_led_list;





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/