CHROMIUM: gobi: Fix races in qc_deregister() once and for all.

CHROMIUM: gobi: Fix races in qc_deregister() once and for all.

This is a fix to my rewritten driver, and is ported from the fix at http://codereview.chromium.org/6602058/.

Signed-off-by: Elly Jones <ellyjones@chromium.org>;
Signed-off-by: Jason Glasgow <jglasgow@chromium.org>;
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>;

BUG=chromium-os:10342
TEST=/opt/Qualcomm/bin/open-deauth

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=41b9938

[Jason Glasgow, 2011-03-09 23:47:55]

I'm concerned.

http://codereview.chromium.org/6612045/diff/1/drivers/net/usb/gobi/qcusbnet.c
File drivers/net/usb/gobi/qcusbnet.c (right):

http://codereview.chromium.org/6612045/diff/1/drivers/net/usb/gobi/qcusbnet.c...
drivers/net/usb/gobi/qcusbnet.c:47: mutex_lock(&dev->lock);
What if the other thread "wins" the race and sets dev->dying = 1, but than does
the qcusbnet_put and frees the device before we get a chance to run mutex_lock? 
Then we try to mutex_lock a freed object.

Am I missing something?  This idiom does not seem correct.

http://codereview.chromium.org/6612045/diff/1/drivers/net/usb/gobi/qcusbnet.c...
drivers/net/usb/gobi/qcusbnet.c:216: kfree(usbnet->net->netdev_ops);
Like Olof said, kfree(NULL)?

[Elly Fong-Jones, 2011-03-10 18:29:13]

Hm. I have a theoretical fix for the race you pointed out. Thoughts?

http://codereview.chromium.org/6612045/diff/1/drivers/net/usb/gobi/qcusbnet.c
File drivers/net/usb/gobi/qcusbnet.c (right):

http://codereview.chromium.org/6612045/diff/1/drivers/net/usb/gobi/qcusbnet.c...
drivers/net/usb/gobi/qcusbnet.c:47: mutex_lock(&dev->lock);
On 2011/03/09 23:47:55, Jason Glasgow wrote:
> What if the other thread "wins" the race and sets dev->dying = 1, but than
does
> the qcusbnet_put and frees the device before we get a chance to run
mutex_lock? 
> Then we try to mutex_lock a freed object.
> 
> Am I missing something?  This idiom does not seem correct.

There is something to what you say. I will meditate upon this.

http://codereview.chromium.org/6612045/diff/1/drivers/net/usb/gobi/qcusbnet.c...
drivers/net/usb/gobi/qcusbnet.c:216: kfree(usbnet->net->netdev_ops);
On 2011/03/09 23:47:55, Jason Glasgow wrote:
> Like Olof said, kfree(NULL)?

Argh.

[Jason Glasgow, 2011-03-10 18:36:28]

Looking better

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qcusbnet.c
File drivers/net/usb/gobi/qcusbnet.c (right):

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qcusbne...
drivers/net/usb/gobi/qcusbnet.c:221: /* See the comments in qcusbnet_get() for
an explanation of why we need
comment should disappear.

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qcusbne...
drivers/net/usb/gobi/qcusbnet.c:225: usbnet->net->netdev_ops = NULL;
still wrong.

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qmidevi...
File drivers/net/usb/gobi/qmidevice.c (right):

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qmidevi...
drivers/net/usb/gobi/qmidevice.c:989: if (handle->dev->dying) {
Dying never set to true.

[Elly Fong-Jones, 2011-03-10 19:15:46]

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qcusbnet.c
File drivers/net/usb/gobi/qcusbnet.c (right):

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qcusbne...
drivers/net/usb/gobi/qcusbnet.c:221: /* See the comments in qcusbnet_get() for
an explanation of why we need
On 2011/03/10 18:36:28, Jason Glasgow wrote:
> comment should disappear.

Done.

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qcusbne...
drivers/net/usb/gobi/qcusbnet.c:225: usbnet->net->netdev_ops = NULL;
On 2011/03/10 18:36:28, Jason Glasgow wrote:
> still wrong.

Done.

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qmidevi...
File drivers/net/usb/gobi/qmidevice.c (right):

http://codereview.chromium.org/6612045/diff/3001/drivers/net/usb/gobi/qmidevi...
drivers/net/usb/gobi/qmidevice.c:989: if (handle->dev->dying) {
On 2011/03/10 18:36:28, Jason Glasgow wrote:
> Dying never set to true.

Oops. Supposed to be done early in qc_deregister(); fixed.

[Mandeep Singh Baines, 2011-03-10 21:18:37]

Hmm, shouldn't this be done by implementing release()?


See Documentation/filesystems/vfs.txt

[Elly Fong-Jones, 2011-03-10 21:35:30]

On 2011/03/10 21:18:37, Mandeep Singh Baines wrote:
> Hmm, shouldn't this be done by implementing release()?
> 
> 
> See Documentation/filesystems/vfs.txt

I don't understand what that has to do with this. It's true that release() needs
to get implemented (and will by a separate CL I'm about to put up) but that
seems orthogonal to the problem here.

[Mandeep Singh Baines, 2011-03-14 18:38:08]

On 2011/03/10 21:35:30, Elly Jones wrote:
> On 2011/03/10 21:18:37, Mandeep Singh Baines wrote:
> > Hmm, shouldn't this be done by implementing release()?
> > 
> > 
> > See Documentation/filesystems/vfs.txt
> 
> I don't understand what that has to do with this. It's true that release()
needs
> to get implemented (and will by a separate CL I'm about to put up) but that
> seems orthogonal to the problem here.

Why are you decrementing the reference count in flush? Shouldn't it be done in
release?

[Mandeep Singh Baines, 2011-03-14 18:58:33]

On 2011/03/14 18:38:08, Mandeep Singh Baines wrote:
> On 2011/03/10 21:35:30, Elly Jones wrote:
> > On 2011/03/10 21:18:37, Mandeep Singh Baines wrote:
> > > Hmm, shouldn't this be done by implementing release()?
> > > 
> > > 
> > > See Documentation/filesystems/vfs.txt
> > 
> > I don't understand what that has to do with this. It's true that release()
> needs
> > to get implemented (and will by a separate CL I'm about to put up) but that
> > seems orthogonal to the problem here.
> 
> Why are you decrementing the reference count in flush? Shouldn't it be done in
> release?

http://www.linuxdriver.co.il/ldd3/chp-3-sect-3.shtml
http://www.linuxdriver.co.il/ldd3/chp-3-sect-5.shtml

[Elly Fong-Jones, 2011-03-14 19:20:26]

On 2011/03/14 18:38:08, Mandeep Singh Baines wrote:
> On 2011/03/10 21:35:30, Elly Jones wrote:
> > On 2011/03/10 21:18:37, Mandeep Singh Baines wrote:
> > > Hmm, shouldn't this be done by implementing release()?
> > > 
> > > 
> > > See Documentation/filesystems/vfs.txt
> > 
> > I don't understand what that has to do with this. It's true that release()
> needs
> > to get implemented (and will by a separate CL I'm about to put up) but that
> > seems orthogonal to the problem here.
> 
> Why are you decrementing the reference count in flush? Shouldn't it be done in
> release?

Fixing that bug in this driver (i.e., using release instead of flush) seems to
me like it ought to be a separate CL?

[Mandeep Singh Baines, 2011-03-14 19:32:46]

On 2011/03/14 19:20:26, Elly Jones wrote:
> On 2011/03/14 18:38:08, Mandeep Singh Baines wrote:
> > On 2011/03/10 21:35:30, Elly Jones wrote:
> > > On 2011/03/10 21:18:37, Mandeep Singh Baines wrote:
> > > > Hmm, shouldn't this be done by implementing release()?
> > > > 
> > > > 
> > > > See Documentation/filesystems/vfs.txt
> > > 
> > > I don't understand what that has to do with this. It's true that release()
> > needs
> > > to get implemented (and will by a separate CL I'm about to put up) but
that
> > > seems orthogonal to the problem here.
> > 
> > Why are you decrementing the reference count in flush? Shouldn't it be done
in
> > release?
> 
> Fixing that bug in this driver (i.e., using release instead of flush) seems to
> me like it ought to be a separate CL?

Maybe do that first? Otherwise, its hard to reason about the reference counts.
You get one release() call per open() but you can get many flush() calls per
open.

[Elly Fong-Jones, 2011-03-15 16:44:38]

On 2011/03/14 19:32:46, Mandeep Singh Baines wrote:
> On 2011/03/14 19:20:26, Elly Jones wrote:
> > On 2011/03/14 18:38:08, Mandeep Singh Baines wrote:
> > > On 2011/03/10 21:35:30, Elly Jones wrote:
> > > > On 2011/03/10 21:18:37, Mandeep Singh Baines wrote:
> > > > > Hmm, shouldn't this be done by implementing release()?
> > > > > 
> > > > > 
> > > > > See Documentation/filesystems/vfs.txt
> > > > 
> > > > I don't understand what that has to do with this. It's true that
release()
> > > needs
> > > > to get implemented (and will by a separate CL I'm about to put up) but
> that
> > > > seems orthogonal to the problem here.
> > > 
> > > Why are you decrementing the reference count in flush? Shouldn't it be
done
> in
> > > release?
> > 
> > Fixing that bug in this driver (i.e., using release instead of flush) seems
to
> > me like it ought to be a separate CL?
> 
> Maybe do that first? Otherwise, its hard to reason about the reference counts.
> You get one release() call per open() but you can get many flush() calls per
> open.

Alright. http://codereview.chromium.org/6694028/ ?

[Elly Fong-Jones, 2011-03-16 19:08:00]

msb: Easier to read now?

[Mandeep Singh Baines, 2011-03-16 20:49:41]

One nit. One question.

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qcusbn...
File drivers/net/usb/gobi/qcusbnet.c (right):

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qcusbn...
drivers/net/usb/gobi/qcusbnet.c:56: struct qcusbnet *entry = NULL;
Why initialize this?

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qmidev...
File drivers/net/usb/gobi/qmidevice.c (right):

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qmidev...
drivers/net/usb/gobi/qmidevice.c:985: if (handle->dev->dying) {
Why isn't device_valid sufficient?

[Elly Fong-Jones, 2011-03-16 21:23:08]

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qcusbn...
File drivers/net/usb/gobi/qcusbnet.c (right):

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qcusbn...
drivers/net/usb/gobi/qcusbnet.c:56: struct qcusbnet *entry = NULL;
On 2011/03/16 20:49:41, Mandeep Singh Baines wrote:
> Why initialize this?

Done.

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qmidev...
File drivers/net/usb/gobi/qmidevice.c (right):

http://codereview.chromium.org/6612045/diff/14001/drivers/net/usb/gobi/qmidev...
drivers/net/usb/gobi/qmidevice.c:985: if (handle->dev->dying) {
On 2011/03/16 20:49:41, Mandeep Singh Baines wrote:
> Why isn't device_valid sufficient?

To avoid racing against the loop in qc_deregister() that frees all the clients.
Basically, if someone can set up a new client and then have the ->valid flag set
to false with that client still live, it can never be freed, because all our
internal functions bail if ->valid is false, including the ones we'd need to
tear down the client context. Fixing this is on my TODO list for the future, but
is not a small task.

[Mandeep Singh Baines, 2011-03-16 21:30:19]

LGTM

[Elly Fong-Jones, 2011-03-16 22:11:31]

On 2011/03/16 21:30:19, Mandeep Singh Baines wrote:
> LGTM

Cool. jglasgow?

[Jason Glasgow, 2011-03-17 17:48:45]

LGTM