Prev: [PATCH 4/4 RFC] firewire: cdev: add PHY packet reception
Next: Remove REDWOOD_[456] config options and conditional code
From: Marcin Slusarz on 16 Jul 2010 16:40
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(a)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(a)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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH 4/4 RFC] firewire: cdev: add PHY packet reception
Next: Remove REDWOOD_[456] config options and conditional code