Date:	Wed, 20 Dec 2006 14:24:26 -0800 (PST)
From:	J <jhnlmn@...oo.com>
To:	Oliver Neukum <oliver@...kum.org>,
	linux-usb-devel@...ts.sourceforge.net
Cc:	linux-kernel@...r.kernel.org
Subject: Re: Possible race condition in usb-serial.c

> Please send in a patch for 2.4. It's very important
> to have a
> very reliable ultraconservative tested kernel
> available.

I will try later. I am new to Linux driver development
and never submitted any patches before.
Also I am not yet 100% sure about the correct way
to solve this issue. I will have to study more.

> I suppose the last time I looked at that code, khubd
> still took BKL. Or I overlooked the race. 

Well, as I already mentioned, BKL/lock_kernel appears
to be completely compiled out of my build, so it is
no help for me.
Also, in my opinion, if usb-serial.c depends on
other modules for synchronization, it should have
a big warning in large font about this, otherwise
bugs are doomed to happen. Or, better yet,
it should have it's own proper synchronization
(as we are trying to come up with now).

 
> Look closer. If you build with preemption, taking
> BKL disables preemption.
> BKL is effective only until you schedule. On UP,
> without preemption
> ordinary kernel code will not reenter. You need no
> lock that doesn't guard
> against interrupts unless you schedule under these
> narrow conditions.

In my old 2.4 build CONFIG_SMP is not defined and,
therefore, lock_kernel is compiled out.
I am not sure about the definition of
CONFIG_LOCK_KERNEL in 2.6.
I tried googling for it, but it brings tons of junk.
 
> Could you test the attached patch against 2.6?

Alas, I only have an old 2.4 right now.
May be I will install 2.6 later (in few weeks).
Currently I am just trying to read 2.6 code with my
eyes.

I studied the patch, which you sent.
I don't see obvious errors, but, in my opinion, it is
not completely perfect.

This attempt to mix ref counting and mutex just does
not look right. For example, in few places you
wrap call to usb_serial_put (which decrements ref
count) with the mutex:

 mutex_lock(&table_lock);
 usb_serial_put(port->serial);
 mutex_unlock(&table_lock);

But in other cases it is called without any mutex.

Also there is a theoretical possibility of a deadlock
because some external functions are called when
the mutex is held
(such as serial->type->shutdown, device_unregister,
usb_put_dev, tty_hangup, etc).

What if these functions will call some TTY routines,
which will somehow synchronize with serial_open?
So, for example, usb_serial_disconnect will wait for 
tty_hangup, which will wait for serial_open,
which waits for usb_serial_disconnect.
Now, I see that in the current 2.6 tty_hangup simply
calls schedule_work, but it may change in the future.
And I don't have time to examine what various shutdown
routines in all the driver do.


So, I don't see a quick solution for this.
I guess, a cleaner way would be to modify the code,
which creates and deletes usb_serial structure.
In particular, currently usb_serial_probe is written
as:
create_serial
...
get_free_serial (which inserts serial to serial_table)
...
initializes serial

As the result a partially initialized serial is
inserted into the table. As the result you had to
lock almost the whole body of usb_serial_probe with
table_lock, which may lead to deadlocks.

It would be cleaner to allocate and fully initialize
usb_serial before inserting it into the table in 
one quick call under mutex (or using atomics).

However, I am not very familiar yet with this code,
so I may be wrong.

John


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 
-
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/

Powered by blists - more mailing lists