* ata kmap_atomic abuse
@ 2006-12-11 8:13 Andrew Morton
2006-12-11 16:07 ` Jeff Garzik
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2006-12-11 8:13 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, linux-ide
[ 231.948000] SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
[ 232.232000] ata1.00: configured for UDMA/33
[ 232.404000] WARNING (1) at arch/i386/mm/highmem.c:47 kmap_atomic()
[ 232.404000] [<c01162e6>] kmap_atomic+0xa9/0x1ab
[ 232.404000] [<c0242c81>] ata_scsi_rbuf_get+0x1c/0x30
[ 232.404000] [<c0242caf>] ata_scsi_rbuf_fill+0x1a/0x87
[ 232.404000] [<c0243ab2>] ata_scsiop_mode_sense+0x0/0x309
[ 232.404000] [<c01729d5>] end_bio_bh_io_sync+0x0/0x37
[ 232.404000] [<c02311c6>] scsi_done+0x0/0x16
[ 232.404000] [<c02311c6>] scsi_done+0x0/0x16
[ 232.404000] [<c0242dcc>] ata_scsi_simulate+0xb0/0x13f
[ 232.404000] [<c0140f12>] mempool_free+0x4a/0x4e
[ 232.404000] [<c02311c6>] scsi_done+0x0/0x16
[ 232.404000] [<c0244451>] ata_scsi_queuecmd+0xa5/0xe1
[ 232.404000] [<c0231817>] scsi_dispatch_cmd+0x1b3/0x226
[ 232.404000] [<c0236284>] scsi_request_fn+0x250/0x2c2
[ 232.404000] [<c01bd7e2>] as_completed_request+0x1ce/0x1db
[ 232.404000] [<c01b9d00>] blk_run_queue+0x2a/0x4b
[ 232.404000] [<c0235025>] scsi_next_command+0x25/0x2f
[ 232.404000] [<c023519e>] scsi_end_request+0x8c/0x96
[ 232.404000] [<c023533f>] scsi_io_completion+0x15a/0x329
[ 232.404000] [<c023ec39>] ata_hsm_move+0x719/0x729
[ 232.404000] [<c023a2e1>] sd_rw_intr+0x209/0x233
[ 232.404000] [<c0110000>] wakeup_pmode_return+0x0/0x55
[ 232.404000] [<c0231164>] scsi_finish_command+0x81/0x88
[ 232.404000] [<c01b9c4f>] blk_done_softirq+0x4a/0x55
[ 232.404000] [<c011d564>] __do_softirq+0x35/0x75
[ 232.404000] [<c011d5c6>] do_softirq+0x22/0x26
[ 232.404000] [<c0104f1a>] do_IRQ+0x67/0x81
[ 232.404000] [<c010352f>] common_interrupt+0x23/0x28
[ 232.404000] =======================
It's using KM_USER0 from softirq.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ata kmap_atomic abuse
2006-12-11 8:13 ata kmap_atomic abuse Andrew Morton
@ 2006-12-11 16:07 ` Jeff Garzik
2006-12-11 18:00 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Garzik @ 2006-12-11 16:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tejun Heo, linux-ide
[-- Attachment #1: Type: text/plain, Size: 357 bytes --]
Andrew Morton wrote:
> [ 231.948000] SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
> [ 232.232000] ata1.00: configured for UDMA/33
> [ 232.404000] WARNING (1) at arch/i386/mm/highmem.c:47 kmap_atomic()
> It's using KM_USER0 from softirq.
I thought that was OK if it was kmap_atomic(). Live and learn.
Checked in the attached.
Jeff
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1664 bytes --]
commit 410228c04f8ee84ae6d07f63dec8df039c9bbd4c
Author: Jeff Garzik <jeff@garzik.org>
Date: Mon Dec 11 11:05:53 2006 -0500
[libata] use kmap_atomic(KM_IRQ0) in SCSI simulator
We are inside spin_lock_irqsave(). quoth akpm's debug facility:
[ 231.948000] SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
[ 232.232000] ata1.00: configured for UDMA/33
[ 232.404000] WARNING (1) at arch/i386/mm/highmem.c:47 kmap_atomic()
[ 232.404000] [<c01162e6>] kmap_atomic+0xa9/0x1ab
[ 232.404000] [<c0242c81>] ata_scsi_rbuf_get+0x1c/0x30
[ 232.404000] [<c0242caf>] ata_scsi_rbuf_fill+0x1a/0x87
[ 232.404000] [<c0243ab2>] ata_scsiop_mode_sense+0x0/0x309
[ 232.404000] [<c01729d5>] end_bio_bh_io_sync+0x0/0x37
[ 232.404000] [<c02311c6>] scsi_done+0x0/0x16
[ 232.404000] [<c02311c6>] scsi_done+0x0/0x16
[ 232.404000] [<c0242dcc>] ata_scsi_simulate+0xb0/0x13f
[...]
Signed-off-by: Jeff Garzik <jeff@garzik.org>
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 664e137..a4790be 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1539,7 +1539,7 @@ static unsigned int ata_scsi_rbuf_get(st
struct scatterlist *sg;
sg = (struct scatterlist *) cmd->request_buffer;
- buf = kmap_atomic(sg->page, KM_USER0) + sg->offset;
+ buf = kmap_atomic(sg->page, KM_IRQ0) + sg->offset;
buflen = sg->length;
} else {
buf = cmd->request_buffer;
@@ -1567,7 +1567,7 @@ static inline void ata_scsi_rbuf_put(str
struct scatterlist *sg;
sg = (struct scatterlist *) cmd->request_buffer;
- kunmap_atomic(buf - sg->offset, KM_USER0);
+ kunmap_atomic(buf - sg->offset, KM_IRQ0);
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: ata kmap_atomic abuse
2006-12-11 16:07 ` Jeff Garzik
@ 2006-12-11 18:00 ` Andrew Morton
2006-12-16 16:40 ` Jeff Garzik
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2006-12-11 18:00 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, linux-ide
On Mon, 11 Dec 2006 11:07:06 -0500
Jeff Garzik <jeff@garzik.org> wrote:
> Andrew Morton wrote:
> > [ 231.948000] SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
> > [ 232.232000] ata1.00: configured for UDMA/33
> > [ 232.404000] WARNING (1) at arch/i386/mm/highmem.c:47 kmap_atomic()
>
> > It's using KM_USER0 from softirq.
>
> I thought that was OK if it was kmap_atomic(). Live and learn.
Each kmap_atomic slot is basically a per-cpu global variable. If a CPU is
inside process-level kmap_atomic(KM_USER0) and then runs that softirq,
it'll return from the softirq with its kmap slot pointing at the wrong
page.
> Checked in the attached.
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 664e137..a4790be 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1539,7 +1539,7 @@ static unsigned int ata_scsi_rbuf_get(st
> struct scatterlist *sg;
>
> sg = (struct scatterlist *) cmd->request_buffer;
> - buf = kmap_atomic(sg->page, KM_USER0) + sg->offset;
> + buf = kmap_atomic(sg->page, KM_IRQ0) + sg->offset;
> buflen = sg->length;
> } else {
> buf = cmd->request_buffer;
> @@ -1567,7 +1567,7 @@ static inline void ata_scsi_rbuf_put(str
> struct scatterlist *sg;
>
> sg = (struct scatterlist *) cmd->request_buffer;
> - kunmap_atomic(buf - sg->offset, KM_USER0);
> + kunmap_atomic(buf - sg->offset, KM_IRQ0);
> }
> }
>
KM_IRQ0 can only be used with local interrupts disabled, for the same
reason.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ata kmap_atomic abuse
2006-12-11 18:00 ` Andrew Morton
@ 2006-12-16 16:40 ` Jeff Garzik
2006-12-16 17:20 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Garzik @ 2006-12-16 16:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: Tejun Heo, linux-ide
Andrew Morton wrote:
> On Mon, 11 Dec 2006 11:07:06 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
>
>> Andrew Morton wrote:
>>> [ 231.948000] SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
>>> [ 232.232000] ata1.00: configured for UDMA/33
>>> [ 232.404000] WARNING (1) at arch/i386/mm/highmem.c:47 kmap_atomic()
>>> It's using KM_USER0 from softirq.
>> I thought that was OK if it was kmap_atomic(). Live and learn.
>
> Each kmap_atomic slot is basically a per-cpu global variable. If a CPU is
> inside process-level kmap_atomic(KM_USER0) and then runs that softirq,
> it'll return from the softirq with its kmap slot pointing at the wrong
> page.
>
>> Checked in the attached.
>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 664e137..a4790be 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -1539,7 +1539,7 @@ static unsigned int ata_scsi_rbuf_get(st
>> struct scatterlist *sg;
>>
>> sg = (struct scatterlist *) cmd->request_buffer;
>> - buf = kmap_atomic(sg->page, KM_USER0) + sg->offset;
>> + buf = kmap_atomic(sg->page, KM_IRQ0) + sg->offset;
>> buflen = sg->length;
>> } else {
>> buf = cmd->request_buffer;
>> @@ -1567,7 +1567,7 @@ static inline void ata_scsi_rbuf_put(str
>> struct scatterlist *sg;
>>
>> sg = (struct scatterlist *) cmd->request_buffer;
>> - kunmap_atomic(buf - sg->offset, KM_USER0);
>> + kunmap_atomic(buf - sg->offset, KM_IRQ0);
>> }
>> }
>>
>
> KM_IRQ0 can only be used with local interrupts disabled, for the same
> reason.
Are you making a comment, or pointing out a flaw in the patch?
AFAICS, the code in question is always under spin_lock_irqsave()
Jeff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ata kmap_atomic abuse
2006-12-16 16:40 ` Jeff Garzik
@ 2006-12-16 17:20 ` Andrew Morton
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2006-12-16 17:20 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, linux-ide
On Sat, 16 Dec 2006 11:40:29 -0500
Jeff Garzik <jeff@garzik.org> wrote:
> Andrew Morton wrote:
> > On Mon, 11 Dec 2006 11:07:06 -0500
> > Jeff Garzik <jeff@garzik.org> wrote:
> >
> >> Andrew Morton wrote:
> >>> [ 231.948000] SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
> >>> [ 232.232000] ata1.00: configured for UDMA/33
> >>> [ 232.404000] WARNING (1) at arch/i386/mm/highmem.c:47 kmap_atomic()
> >>> It's using KM_USER0 from softirq.
> >> I thought that was OK if it was kmap_atomic(). Live and learn.
> >
> > Each kmap_atomic slot is basically a per-cpu global variable. If a CPU is
> > inside process-level kmap_atomic(KM_USER0) and then runs that softirq,
> > it'll return from the softirq with its kmap slot pointing at the wrong
> > page.
> >
> >> Checked in the attached.
> >
> >
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> >> index 664e137..a4790be 100644
> >> --- a/drivers/ata/libata-scsi.c
> >> +++ b/drivers/ata/libata-scsi.c
> >> @@ -1539,7 +1539,7 @@ static unsigned int ata_scsi_rbuf_get(st
> >> struct scatterlist *sg;
> >>
> >> sg = (struct scatterlist *) cmd->request_buffer;
> >> - buf = kmap_atomic(sg->page, KM_USER0) + sg->offset;
> >> + buf = kmap_atomic(sg->page, KM_IRQ0) + sg->offset;
> >> buflen = sg->length;
> >> } else {
> >> buf = cmd->request_buffer;
> >> @@ -1567,7 +1567,7 @@ static inline void ata_scsi_rbuf_put(str
> >> struct scatterlist *sg;
> >>
> >> sg = (struct scatterlist *) cmd->request_buffer;
> >> - kunmap_atomic(buf - sg->offset, KM_USER0);
> >> + kunmap_atomic(buf - sg->offset, KM_IRQ0);
> >> }
> >> }
> >>
> >
> > KM_IRQ0 can only be used with local interrupts disabled, for the same
> > reason.
>
> Are you making a comment, or pointing out a flaw in the patch?
<tries to remember>
> AFAICS, the code in question is always under spin_lock_irqsave()
ok. That is in fact obligatory: if we were to call this (as-modified)
function from softirq context but with local IRQs enabled, an intervening
interrupt could come in and make out kmap slot point somewhere else.
We require that local interrupts remain disabled by the caller until the
caller has called ata_scsi_rbuf_put().
This means that we require that all callers of the non-static
ata_scsi_rbuf_fill() have disabled local IRQs. The nice comment says they
must do that, so hopefully we're OK there.
So yes, it all looks to be solid to me.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-12-16 17:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-12-11 8:13 ata kmap_atomic abuse Andrew Morton
2006-12-11 16:07 ` Jeff Garzik
2006-12-11 18:00 ` Andrew Morton
2006-12-16 16:40 ` Jeff Garzik
2006-12-16 17:20 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).