* Re: [PATCH 01/15] alpha: srmcons: localize spin_lock section
2010-07-16 16:12 [PATCH 01/15] alpha: srmcons: localize spin_lock section Kulikov Vasiliy
@ 2010-07-16 20:30 ` Marcin Slusarz
0 siblings, 0 replies; 2+ messages in thread
From: Marcin Slusarz @ 2010-07-16 20:30 UTC (permalink / raw)
To: Kulikov Vasiliy
Cc: kernel-janitors, Richard Henderson, Ivan Kokshaysky, Matt Turner,
linux-alpha, linux-kernel, Andrew Morton
On Fri, Jul 16, 2010 at 08:12:27PM +0400, Kulikov Vasiliy wrote:
> Scale down spin_lock_irqsave/spin_unlock_irqrestore exactly to area
> where it is needed. Also it makes static checkers happy as code checks
> kmalloc() result value exactly after call.
>
> Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
> ---
> arch/alpha/kernel/srmcons.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
> index 783f4e5..6b04005 100644
> --- a/arch/alpha/kernel/srmcons.c
> +++ b/arch/alpha/kernel/srmcons.c
> @@ -165,17 +165,15 @@ srmcons_get_private_struct(struct srmcons_private **ps)
>
> if (srmconsp == NULL) {
> srmconsp = kmalloc(sizeof(*srmconsp), GFP_KERNEL);
> - spin_lock_irqsave(&srmconsp_lock, flags);
> -
> if (srmconsp == NULL)
> retval = -ENOMEM;
> else {
> + spin_lock_irqsave(&srmconsp_lock, flags);
> srmconsp->tty = NULL;
> spin_lock_init(&srmconsp->lock);
> init_timer(&srmconsp->timer);
> + spin_unlock_irqrestore(&srmconsp_lock, flags);
> }
> -
> - spin_unlock_irqrestore(&srmconsp_lock, flags);
> }
>
> *ps = srmconsp;
> --
Locking in this function seems to be broken. If we are unlucky:
- two cpus will allocate, initialize and return two different pointers
- two cpus will allocate, but both initialize only one (two times!) and return
the same pointer (so one of them will be leaked) - 2 initializations may lead
to double unlock later, etc
- ...
Something like patch below is needed. I don't have alpha cross compiler so
I can't even compile it...
---
From: Marcin Slusarz <marcin.slusarz@gmail.com>
Subject: [PATCH] alpha: fix races in srmcons_get_private_struct
---
arch/alpha/kernel/srmcons.c | 38 ++++++++++++++++++++++++--------------
1 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/arch/alpha/kernel/srmcons.c b/arch/alpha/kernel/srmcons.c
index 783f4e5..531ac99 100644
--- a/arch/alpha/kernel/srmcons.c
+++ b/arch/alpha/kernel/srmcons.c
@@ -161,25 +161,35 @@ srmcons_get_private_struct(struct srmcons_private **ps)
static struct srmcons_private *srmconsp = NULL;
static DEFINE_SPINLOCK(srmconsp_lock);
unsigned long flags;
- int retval = 0;
+ struct srmcons_private *tmp;
- if (srmconsp == NULL) {
- srmconsp = kmalloc(sizeof(*srmconsp), GFP_KERNEL);
- spin_lock_irqsave(&srmconsp_lock, flags);
-
- if (srmconsp == NULL)
- retval = -ENOMEM;
- else {
- srmconsp->tty = NULL;
- spin_lock_init(&srmconsp->lock);
- init_timer(&srmconsp->timer);
- }
+ if (srmconsp) {
+ *ps = srmconsp;
+ return 0;
+ }
- spin_unlock_irqrestore(&srmconsp_lock, flags);
+ tmp = kmalloc(sizeof(*srmconsp), GFP_KERNEL);
+ if (!tmp) {
+ *ps = NULL;
+ return -ENOMEM;
}
+ spin_lock_irqsave(&srmconsp_lock, flags);
+
+ /* recheck under lock to not race with another cpu */
+ if (srmconsp == NULL) {
+ tmp->tty = NULL;
+ spin_lock_init(&tmp->lock);
+ init_timer(&tmp->timer);
+
+ srmconsp = tmp;
+ } else
+ kfree(tmp);
+
+ spin_unlock_irqrestore(&srmconsp_lock, flags);
+
*ps = srmconsp;
- return retval;
+ return 0;
}
static int
--
1.7.1
^ permalink raw reply [flat|nested] 2+ messages in thread