| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 8 Jul 2011 03:23:36 +0530
From: <Shyam_Iyer@...l.com>
To: <ivecera@...hat.com>, <shyam.iyer.t@...il.com>
CC: <netdev@...r.kernel.org>, <rmody@...cade.com>, <ddutt@...cade.com>,
<huangj@...cade.com>, <davem@...emloft.net>
Subject: RE: [PATCH] [v3][net][bna] Fix call trace when interrupts are
disabled while sleeping function kzalloc is called
> -----Original Message-----
> From: Ivan Vecera [mailto:ivecera@...hat.com]
> Sent: Thursday, July 07, 2011 11:14 AM
> To: Shyam Iyer
> Cc: netdev@...r.kernel.org; rmody@...cade.com; ddutt@...cade.com;
> huangj@...cade.com; davem@...emloft.net; Iyer, Shyam
> Subject: Re: [PATCH] [v3][net][bna] Fix call trace when interrupts are
> disabled while sleeping function kzalloc is called
>
> Small note, the root of the problem was that non-atomic allocation was
> requested with IRQs disabled. Your patch description does not contain
> why were the IRQs disabled.
> The function bnad_mbox_irq_alloc incorrectly uses 'flags' var for two
> different things, 1) to save current CPU flags and 2) for request_irq
> call.
> First the spin_lock_irqsave disables the IRQs and saves _all_ CPU flags
> (including one that enables/disables interrupts) to 'flags'. Then the
> 'flags' is overwritten by 0 or 0x80 (IRQF_SHARED). Finally the
> spin_unlock_irqrestore should restore saved flags, but these flags are
> now either 0x00 or 0x80. The interrupt bit value in flags register on
> x86 arch is 0x100.
> This means that the interrupt bit is zero (IRQs disabled) after
> spin_unlock_irqrestore so the request_irq function is called with
> disabled interrupts.
That seems to make a lot more sense.. and that way I don't have to initialize the flags variable outside of the spin_lock_irqsave/restore bracket to preserve the irqs not being disabled when allocating in request_irq
Would the below patch make sense instead...? (Note that since davem has accepted the earlier one this is on top of the already committed patch)
diff --git a/drivers/net/bna/bnad.c b/drivers/net/bna/bnad.c
index 44e219c..ea13605 100644
--- a/drivers/net/bna/bnad.c
+++ b/drivers/net/bna/bnad.c
@@ -1111,7 +1111,7 @@ bnad_mbox_irq_alloc(struct bnad *bnad,
struct bna_intr_info *intr_info)
{
int err = 0;
- unsigned long irq_flags = 0, flags;
+ unsigned long irq_flags, flags;
u32 irq;
irq_handler_t irq_handler;
@@ -1125,6 +1125,7 @@ bnad_mbox_irq_alloc(struct bnad *bnad,
if (bnad->cfg_flags & BNAD_CF_MSIX) {
irq_handler = (irq_handler_t)bnad_msix_mbox_handler;
irq = bnad->msix_table[bnad->msix_num - 1].vector;
+ irq_flags = 0;
intr_info->intr_type = BNA_INTR_T_MSIX;
intr_info->idl[0].vector = bnad->msix_num - 1;
} else {
@@ -1135,7 +1136,6 @@ bnad_mbox_irq_alloc(struct bnad *bnad,
/* intr_info->idl.vector = 0 ? */
}
spin_unlock_irqrestore(&bnad->bna_lock, flags);
- flags = irq_flags;
sprintf(bnad->mbox_irq_name, "%s", BNAD_NAME);
/*
@@ -1146,7 +1146,7 @@ bnad_mbox_irq_alloc(struct bnad *bnad,
BNAD_UPDATE_CTR(bnad, mbox_intr_disabled);
- err = request_irq(irq, irq_handler, flags,
+ err = request_irq(irq, irq_handler, irq_flags,
bnad->mbox_irq_name, bnad);
if (err) {
(END)
>
> Rasesh, this is not a good idea to use one variable for two different
> purposes in parallel.
>
> Regards,
> Ivan
>
> On Tue, 2011-06-28 at 14:58 -0400, Shyam Iyer wrote:
> > request_threaded irq will call kzalloc that can sleep. Initializing
> the flags variable outside of spin_lock_irqsave/restore in
> bnad_mbox_irq_alloc will avoid call traces like below.
> >
> > Jun 27 08:15:24 home-t710 kernel: [11735.634550] Brocade 10G Ethernet
> driver
> > Jun 27 08:15:24 home-t710 kernel: [11735.634590] bnad_pci_probe :
> (0xffff880427f3d000, 0xffffffffa020f3e0) PCI Func : (2)
> > Jun 27 08:15:24 home-t710 kernel: [11735.637677] bna 0000:82:00.2:
> PCI INT A -> GSI 66 (level, low) -> IRQ 66
> > Jun 27 08:15:24 home-t710 kernel: [11735.638290] bar0 mapped to
> ffffc90014980000, len 262144
> > Jun 27 08:15:24 home-t710 kernel: [11735.638732] BUG: sleeping
> function called from invalid context at mm/slub.c:847
> > Jun 27 08:15:24 home-t710 kernel: [11735.638736] in_atomic(): 0,
> irqs_disabled(): 1, pid: 11243, name: insmod
> > Jun 27 08:15:24 home-t710 kernel: [11735.638740] Pid: 11243, comm:
> insmod Not tainted 3.0.0-rc4+ #6
> > Jun 27 08:15:24 home-t710 kernel: [11735.638743] Call Trace:
> > Jun 27 08:15:24 home-t710 kernel: [11735.638755]
> [<ffffffff81046427>] __might_sleep+0xeb/0xf0
> > Jun 27 08:15:24 home-t710 kernel: [11735.638766]
> [<ffffffffa01fe469>] ? netif_wake_queue+0x3d/0x3d [bna]
> > Jun 27 08:15:24 home-t710 kernel: [11735.638773]
> [<ffffffff8111201c>] kmem_cache_alloc_trace+0x43/0xd8
> > Jun 27 08:15:24 home-t710 kernel: [11735.638782]
> [<ffffffffa01fe469>] ? netif_wake_queue+0x3d/0x3d [bna]
> > Jun 27 08:15:24 home-t710 kernel: [11735.638787]
> [<ffffffff810ab791>] request_threaded_irq+0xa1/0x113
> > Jun 27 08:15:24 home-t710 kernel: [11735.638798]
> [<ffffffffa020f0c0>] bnad_pci_probe+0x612/0x8e5 [bna]
> > Jun 27 08:15:24 home-t710 kernel: [11735.638807]
> [<ffffffffa01fe469>] ? netif_wake_queue+0x3d/0x3d [bna]
> > Jun 27 08:15:24 home-t710 kernel: [11735.638816]
> [<ffffffff81482ef4>] ? _raw_spin_unlock_irqrestore+0x17/0x19
> > Jun 27 08:15:24 home-t710 kernel: [11735.638822]
> [<ffffffff8124d17a>] local_pci_probe+0x44/0x75
> > Jun 27 08:15:24 home-t710 kernel: [11735.638826]
> [<ffffffff8124dc06>] pci_device_probe+0xd0/0xff
> > Jun 27 08:15:24 home-t710 kernel: [11735.638832]
> [<ffffffff812ef8ab>] driver_probe_device+0x131/0x213
> > Jun 27 08:15:24 home-t710 kernel: [11735.638836]
> [<ffffffff812ef9e7>] __driver_attach+0x5a/0x7e
> > Jun 27 08:15:24 home-t710 kernel: [11735.638840]
> [<ffffffff812ef98d>] ? driver_probe_device+0x213/0x213
> > Jun 27 08:15:24 home-t710 kernel: [11735.638844]
> [<ffffffff812ee933>] bus_for_each_dev+0x53/0x89
> > Jun 27 08:15:24 home-t710 kernel: [11735.638848]
> [<ffffffff812ef48a>] driver_attach+0x1e/0x20
> > Jun 27 08:15:24 home-t710 kernel: [11735.638852]
> [<ffffffff812ef0ae>] bus_add_driver+0xd1/0x224
> > Jun 27 08:15:24 home-t710 kernel: [11735.638858]
> [<ffffffffa01b8000>] ? 0xffffffffa01b7fff
> > Jun 27 08:15:24 home-t710 kernel: [11735.638862]
> [<ffffffff812efe57>] driver_register+0x98/0x105
> > Jun 27 08:15:24 home-t710 kernel: [11735.638866]
> [<ffffffffa01b8000>] ? 0xffffffffa01b7fff
> > Jun 27 08:15:24 home-t710 kernel: [11735.638871]
> [<ffffffff8124e4c9>] __pci_register_driver+0x56/0xc1
> > Jun 27 08:15:24 home-t710 kernel: [11735.638875]
> [<ffffffffa01b8000>] ? 0xffffffffa01b7fff
> > Jun 27 08:15:24 home-t710 kernel: [11735.638884]
> [<ffffffffa01b8040>] bnad_module_init+0x40/0x60 [bna]
> > Jun 27 08:15:24 home-t710 kernel: [11735.638892]
> [<ffffffff81002099>] do_one_initcall+0x7f/0x136
> > Jun 27 08:15:24 home-t710 kernel: [11735.638899]
> [<ffffffff8108608b>] sys_init_module+0x88/0x1d0
> > Jun 27 08:15:24 home-t710 kernel: [11735.638906]
> [<ffffffff81489682>] system_call_fastpath+0x16/0x1b
> > Jun 27 08:15:24 home-t710 kernel: [11735.639642] bnad_pci_probe :
> (0xffff880427f3e000, 0xffffffffa020f3e0) PCI Func : (3)
> > Jun 27 08:15:24 home-t710 kernel: [11735.639665] bna 0000:82:00.3:
> PCI INT A -> GSI 66 (level, low) -> IRQ 66
> > Jun 27 08:15:24 home-t710 kernel: [11735.639735] bar0 mapped to
> ffffc90014400000, len 262144
> >
> > Signed-off-by: Shyam Iyer <shyam_iyer@...l.com>
> > ---
> > drivers/net/bna/bnad.c | 7 +++----
> > 1 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/bna/bnad.c b/drivers/net/bna/bnad.c
> > index 7d25a97..44e219c 100644
> > --- a/drivers/net/bna/bnad.c
> > +++ b/drivers/net/bna/bnad.c
> > @@ -1111,7 +1111,7 @@ bnad_mbox_irq_alloc(struct bnad *bnad,
> > struct bna_intr_info *intr_info)
> > {
> > int err = 0;
> > - unsigned long flags;
> > + unsigned long irq_flags = 0, flags;
> > u32 irq;
> > irq_handler_t irq_handler;
> >
> > @@ -1125,18 +1125,17 @@ bnad_mbox_irq_alloc(struct bnad *bnad,
> > if (bnad->cfg_flags & BNAD_CF_MSIX) {
> > irq_handler = (irq_handler_t)bnad_msix_mbox_handler;
> > irq = bnad->msix_table[bnad->msix_num - 1].vector;
> > - flags = 0;
> > intr_info->intr_type = BNA_INTR_T_MSIX;
> > intr_info->idl[0].vector = bnad->msix_num - 1;
> > } else {
> > irq_handler = (irq_handler_t)bnad_isr;
> > irq = bnad->pcidev->irq;
> > - flags = IRQF_SHARED;
> > + irq_flags = IRQF_SHARED;
> > intr_info->intr_type = BNA_INTR_T_INTX;
> > /* intr_info->idl.vector = 0 ? */
> > }
> > spin_unlock_irqrestore(&bnad->bna_lock, flags);
> > -
> > + flags = irq_flags;
> > sprintf(bnad->mbox_irq_name, "%s", BNAD_NAME);
> >
> > /*
>
>
Powered by blists - more mailing lists