[RFC] watchdog ->release() races

[RFC] watchdog ->release() races

[Al Viro]

	Watchdog drivers tend to do something like that:

foo_open()
{
	if (test_and_set_bit(0, &foo_is_open))
		return -EBUSY;
	...
}

foo_write()
{
	...
	assign foo_expect_close
	...
}

foo_release()
{
	look at foo_expect_close, act accordingly
	clear_bit(0, &foo_is_open);
	foo_expect_close = 0;
}

OK, so it tries to make sure that there's only one opened struct file for
the device; fair enough, but what happens if we have
task A: open()/write()/close()
task B: open()/write()/close()
with task A losing CPU just between clear_bit() and clearing foo_expect_close?
If it regains CPU just after write() done by task B, we'll get foo_expect_close
unexpectedly cleared.

It's obviously racy; I'm not sure if we care about that race, but if we
do, there's about 80 drivers that need to be fixed...

Comments?

Re: [RFC] watchdog ->release() races

[Linus Torvalds]

On Tue, May 7, 2013 at 1:10 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> It's obviously racy; I'm not sure if we care about that race, but if we
> do, there's about 80 drivers that need to be fixed...

I suspect the "we don't really care" is the correct answer here. It's
not like you can really screw things up by mistake, and if you try to
screw this up on purpose, you need to be root anyway to open
/dev/watchdog etc.

               Linus

Re: [RFC] watchdog ->release() races

[Al Viro]

On Tue, May 07, 2013 at 02:00:02PM -0700, Linus Torvalds wrote:
> On Tue, May 7, 2013 at 1:10 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > It's obviously racy; I'm not sure if we care about that race, but if we
> > do, there's about 80 drivers that need to be fixed...
> 
> I suspect the "we don't really care" is the correct answer here. It's
> not like you can really screw things up by mistake, and if you try to
> screw this up on purpose, you need to be root anyway to open
> /dev/watchdog etc.

I'm not sure...  It's obviously not a "the sky is falling" kind of thing,
but in a situation when task B kills task A, then spins trying to open
until open() succeeds...  Hell knows; it might be triggerable by somebody
not trying to trigger the race on purpose.  I really don't know what does
the userland side of that thing look like, thus the Cc to linux-wtchdog...

Re: [RFC] watchdog ->release() races

[Guenter Roeck]

On Tue, May 07, 2013 at 09:10:01PM +0100, Al Viro wrote:
> 	Watchdog drivers tend to do something like that:
> 
> foo_open()
> {
> 	if (test_and_set_bit(0, &foo_is_open))
> 		return -EBUSY;
> 	...
> }
> 
> foo_write()
> {
> 	...
> 	assign foo_expect_close
> 	...
> }
> 
> foo_release()
> {
> 	look at foo_expect_close, act accordingly
> 	clear_bit(0, &foo_is_open);
> 	foo_expect_close = 0;
> }
> 
> OK, so it tries to make sure that there's only one opened struct file for
> the device; fair enough, but what happens if we have
> task A: open()/write()/close()
> task B: open()/write()/close()
> with task A losing CPU just between clear_bit() and clearing foo_expect_close?
> If it regains CPU just after write() done by task B, we'll get foo_expect_close
> unexpectedly cleared.
> 
> It's obviously racy; I'm not sure if we care about that race, but if we
> do, there's about 80 drivers that need to be fixed...
> 
> Comments?

Good catch.

Unless I am missing something, the problem should be fixed for drivers using
the watchdog infrastructure. Might be a good incentive to convert the remaining
drivers.

There is a race in watchdog_register_device which I think is a bit more serious.
See https://patchwork.kernel.org/patch/2400801/.

It is in linux-next, so Linus should get it from Wim's pull request.

Guenter