[PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable. variable.

[PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable. variable.

[Mark Balantzyan]

---
 drivers/watchdog/alim1535_wdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..4ba2b860 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -107,6 +107,7 @@ static void ali_keepalive(void)
 
 static int ali_settimer(int t)
 {
+    spin_lock(&ali_lock);
     if (t < 0)
         return -EINVAL;
     else if (t < 60)
@@ -117,7 +118,7 @@ static int ali_settimer(int t)
         ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
     else
         return -EINVAL;
-
+    spin_unlock(&ali_lock);
     timeout = t;
     return 0;
 }
-- 
2.17.1
Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>
Cc: Pavel Andrianov <andrianov@ispras.ru>
Cc:Wim Van Sebroeck <wim@linux-watchdog.org> (maintainer:WATCHDOG DEVICE DRIVERS)
Cc: Guenter Roeck <linux@roeck-us.net> (maintainer:WATCHDOG DEVICE DRIVERS)
Cc:linux-watchdog@vger.kernel.org (open list:WATCHDOG DEVICE DRIVERS)
Cc:linux-kernel@vger.kernel.org (open list)

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable. variable.

[Guenter Roeck]

On Thu, Jul 18, 2019 at 08:52:38AM -0700, Mark Balantzyan wrote:
> ---

Subject and description are all messed up.

>  drivers/watchdog/alim1535_wdt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
> index 60f0c2eb..4ba2b860 100644
> --- a/drivers/watchdog/alim1535_wdt.c
> +++ b/drivers/watchdog/alim1535_wdt.c
> @@ -107,6 +107,7 @@ static void ali_keepalive(void)
>  
>  static int ali_settimer(int t)
>  {
> +    spin_lock(&ali_lock);
>      if (t < 0)
>          return -EINVAL;
>      else if (t < 60)
> @@ -117,7 +118,7 @@ static int ali_settimer(int t)
>          ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
>      else
>          return -EINVAL;

This return and the return above will exit the function with the
spinlock still active, which will guarantee a hangup if/when the
function is re-entered.

> -
> +    spin_unlock(&ali_lock);
>      timeout = t;

timeout is still unprotected and may have no relation to the
stored value of ali_timeout_bits.

Overall your patch would introduce much more severe problems
than the problem it tries to fix, and it doesn't even completely
fix that problem either.

I would suggest to leave the driver alone, unless you have the hardware
to test your changes. And, if you do, it would be much more valuable
to convert the driver to use the watchdog subsystem.

Thanks,
Guenter

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Mark Balantzyan]

Hi Guenter,

If it's not too much too ask, I also propose to rewrite alim1535_wdt to 
use the watchdog subsystem as I believe we are making progress toward the 
similar end in pc87413_wdt, as my evaluation ends in some weeks.

Thank you,
Mark

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Guenter Roeck]

Hi Mark,

On Wed, Jul 31, 2019 at 09:17:13AM -0700, Mark Balantzyan wrote:
> Hi Guenter,
> 
> If it's not too much too ask, I also propose to rewrite alim1535_wdt to use
> the watchdog subsystem as I believe we are making progress toward the
> similar end in pc87413_wdt, as my evaluation ends in some weeks.
> 

Please, no. We still have ways to go with that one driver, and we'll be
stuck with a patch which I can't accept because of lack of testing.
I (and you) really need to talk to your evaluators why they ask you
to make those changes. This is highly inappropriate. The Linux kernel
is not an feasible target for such an evaluation. This is setting
you up for failure, and it is a waste of both your and my time.

Are you really working for or on belalf of the Linux Foundation ?
They should know better. And if Google is involved, I am embarassed
for my employer.  If they really want people to do work like this,
they should also provide reviewers and coaching staff. They should
most definitely not expect kernel maintainers to do it for them.

Thanks,
Guenter

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Mark Balantzyan]

My employer (and yes, I am working for the Linux Foundation) has me 
working on analysing race condition warnings in the Linux kernel. They 
have a driver verification project running under the umbrella of the ELISA 
project involved in the research, investigation, experimentation, and 
establishment of linux kernel verification measures and tools.

I actually do have assigned to me coaches and/or mentors that I have been 
corresponding with. They are aware of what is going on and are being cc'd 
to (most of) our emails.

pc87413_wdt was detected by our race condition analysis tool as having 
warning. Even outside this work we've been doing, I've been trying to apply
the reasoning of the race condition analysis tool to different kernel modules,
as part of my menteeship.

I hope you can respect that this is a process primarily for learning and 
experimentation. I'm sorry if I'm creating too much work for you at once. 
If so, let me know and I'll try to spread it out.

Thank you,
Mark

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Mark Balantzyan]

Hi Guenter, all,

It's alright if you still don't wish to review my patch on alim1535_wdt, 
but my employer and I, using our race condition analysis tool, detected it 
to contain a race condition warning. I believe any possible issues could 
be resolved if it were rewritten to use the watchdog subsystem as you've 
previously stated.

Now, I don't wish to bother you too much, but it seems I forgot to work 
mainly with my assigned mentor prior to submitting patches..sorry. So, 
after I have worked on rewriting the alim1535 driver into common watchdog 
subsystem with my mentor, may I submit it to you then?

Thank you,
Mark

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Guenter Roeck]

Mark,

On Wed, Jul 31, 2019 at 11:23:19AM -0700, Mark Balantzyan wrote:
> Hi Guenter, all,
> 
> It's alright if you still don't wish to review my patch on alim1535_wdt, but
> my employer and I, using our race condition analysis tool, detected it to
> contain a race condition warning. I believe any possible issues could be
> resolved if it were rewritten to use the watchdog subsystem as you've
> previously stated.
> 
> Now, I don't wish to bother you too much, but it seems I forgot to work
> mainly with my assigned mentor prior to submitting patches..sorry. So, after
> I have worked on rewriting the alim1535 driver into common watchdog
> subsystem with my mentor, may I submit it to you then?
> 

Similar to pc87413, this driver very likely has zero users left, there
won't be any hardware to test the patch, and we won't be able to accept
such a patch because it wasn't tested.

On top of that, the only race condition I can see in that driver is in
ali_settimer(), between ali_timeout_bits and timeout. Yet, that is not
really a race condition because the driver can only be opened once,
and thus there is no means for two threads entering ali_settimer()
at the same time.

I don't really understand this focus on fixing theoretic/irrelevant
race conditions in drivers which no one uses anymore. Maybe someone
can enlighten me ?

Thanks,
Guenter

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Mark Balantzyan]

Hi Guenter, all,

 	I don't really understand this focus on fixing theoretic/irrelevant
 	race conditions in drivers which no one uses anymore. Maybe someone
 	can enlighten me ?

In conjunction with linuxtesting.org and The Linux Foundation, I've been 
enlisted to test and work on helping to test tools they use for 
reliability testing of linux-based systems. In particular, two tools, 
RaceHound (whose command is 'lines2insns' and which isolates race 
conditions in kernel modules) and Klever, which is browser-based, are
under continual development by the Center and I aim to help them improve
their throughput by aiding in investigating where, in the automated nature
particularly of Klever (requiring considerable configuration as well),
there may areas to improve.

Hence, yes, a large amount of our findings result in manifesting as 
theoretical and possible only, but relevant to improving the tools 
nonetheless.

Hope that helps with the 'enlightening' :), regards,
Mark

Re: [PATCH] watchdog:alim1535_wdt: Fix data race in ali_settimer() concerning ali_timeout_bits variable.

[Guenter Roeck]

On Mon, Jul 22, 2019 at 07:35:03AM -0700, Mark Balantzyan wrote:
> Hello all,
> 
> I had previously submitted 2 patches attempting to fix the data race
> condition in alim1535_wdt.c as part of my work with individuals on the
> Linux Driver Verification project. I am including the original patch
> description I provided, below, along with revised patch. Thank you.
> 
> (PATCH DESCRIPTION AND PATCH BELOW)
> 
This is not how the description is supposed to look like; the above would
end up in the commit log. Please check the kernel documentation on how
to write subject lines and patch descriptions.

> In drivers/watchdog/ailm1535_wdt.c, there is the potential for a data race
> due to parallel call stacks as follows: Thread 1 calls a file operation
> callback, denoted *ali_fops* in the .c file, which in turn results in calls
> to ali_write() followed by ali_start(), which has the line
> 
> val |= (1 << 25) | ali_timeout_bits;
> 
> surrounded by a spin_lock and spin_unlock. This is crucial because Thread 2

The "surrounded by spinlock" does not refer to the line above, but to
pci_read_config_dword() followed by pci_write_config_dword(), which
needs to be protected. The described race condition around ali_timeout_bits
[ali_start() vs. ali_settimer()] does not exist.

The only race condition in the driver is 'ali_timeout_bits' vs. 'timeout'.
It is theoretically possible that those two get out of sync, ie that
ali_timeout_bits does not reflect the value of timeout. This can happen
if one of the threads is interrupted after setting 'ali_timeout_bits' but
before updating 'timeout'.

> can access "ali_timeout_bits" then when it calls ali_ioctl(), which calls
> ali_settimer() having the lines (else if (t < 60) ali_timeout_bits = t|(1
>  << 6);, lines 112-113, etc.)
> 
There is no need to be that detailed. It is sufficient to explain that
there is a race condition when updating 'ali_timeout_bits' and 'timeout'
(or maybe use my explanation above).

> (Revised) patch adds spinlocking around "ali_timeout_bits" in
> ali_settimer() should "ali_ioctl()" be called in a concurrent thread (at
> any time).
> ---
>  drivers/watchdog/alim1535_wdt.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/alim1535_wdt.c
> b/drivers/watchdog/alim1535_wdt.c
> index 60f0c2e..1260e9e 100644
> --- a/drivers/watchdog/alim1535_wdt.c
> +++ b/drivers/watchdog/alim1535_wdt.c
> @@ -106,19 +106,23 @@ static void ali_keepalive(void)
>   */
> 
>  static int ali_settimer(int t)
> -{
> - if (t < 0)
> +{ spin_lock(&ali_lock);
> + if (t < 0) {
> + spin_unlock(&ali_unlock);
>   return -EINVAL;
> + }
>   else if (t < 60)
>   ali_timeout_bits = t|(1 << 6);
>   else if (t < 3600)
>   ali_timeout_bits = (t / 60)|(1 << 7);
>   else if (t < 18000)
>   ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
> - else
> + else {
> + spin_unlock(&ali_lock);
>   return -EINVAL;

Please use goto for error exits, as suggested in the coding style document.

> -
> + }
>   timeout = t;
> + spin_unlock(&ali_lock);
>   return 0;
>  }

Formatting is completely messed up.

> 
> -- 
> 2.15.1
> Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>

Wrong location for Signed-off-by:.

Guenter