* 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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.