Possible bug in cypress_m8.ko

Possible bug in cypress_m8.ko

[Anton Volkov]

Hello.

Judging by the code of cypress_m8.c some functions are considered to be 
capable of working concurrently with other functions, e.g. cypress_open.
There are, however, entities that are protected by the locks at one 
place and not protected in another. Lines are given using the info from 
Linux kernel v4.12. Example:

cypress_send
   spin_lock_irqsave
   priv->write_urb_in_use = 1;
   spin_lock_irqrestore
   (cypress_m8.c: lines 761-763)
   ...
   if (result) {
      priv->write_urb_in_use = 0; //without lock protection
      (cypress_m8.c: line 783)
   }

Is it a bug?

Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avolkov@ispras.ru

Re: Possible bug in cypress_m8.ko

[Oliver Neukum]

Am Dienstag, den 22.08.2017, 15:11 +0300 schrieb Anton Volkov:
> Hello.
> 
> Judging by the code of cypress_m8.c some functions are considered to be 
> capable of working concurrently with other functions, e.g. cypress_open.
> There are, however, entities that are protected by the locks at one 
> place and not protected in another. Lines are given using the info from 
> Linux kernel v4.12. Example:
> 
> cypress_send
>    spin_lock_irqsave
>    priv->write_urb_in_use = 1;
>    spin_lock_irqrestore
>    (cypress_m8.c: lines 761-763)
>    ...
>    if (result) {
>       priv->write_urb_in_use = 0; //without lock protection
>       (cypress_m8.c: line 783)
>    }
> 
> Is it a bug?

Yes, but not of the kind you describe.
The transition from "not in use" to "in use" must be guarded by
a lock, because it may be contended.
But if that transition is properly guarded, you already know
that there can be only user. He can theoretically give up
the resource without locking.

Yet there is a bug:

^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  702)       spin_lock_irqsave(&priv->lock, flags);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  703)       if (priv->write_urb_in_use) {
441b62c1edb98 (Harvey Harrison    2008-03-03 16:08:34 -0800  704)               dbg("%s - can't write, urb in use", __func__);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  705)               spin_unlock_irqrestore(&priv->lock, flags);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  706)               return;
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  707)       }
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  708)       spin_unlock_irqrestore(&priv->lock, flags);

The flag is checked is checked under a lock. But then the lock is dropped.
And here:

^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  759)       spin_lock_irqsave(&priv->lock, flags);
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  760)       priv->write_urb_in_use = 1;
^1da177e4c3f4 (Linus Torvalds     2005-04-16 15:20:36 -0700  761)       spin_unlock_irqrestore(&priv->lock, flags);

The flag is set under lock, but unconditionally.
The code just makes no sense.


In addition, when you drop the flag without a lock you need to worry
about memory ordering.

	HTH
		Oliver