* [PATCH 2.4] the perils of kunmap_atomic
@ 2004-10-26 23:31 Jeff Garzik
2004-10-26 23:36 ` Nigel Cunningham
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jeff Garzik @ 2004-10-26 23:31 UTC (permalink / raw)
To: Linux Kernel, Marcelo Tosatti
Cc: Bartlomiej Zolnierkiewicz, Douglas Gilbert, Jens Axboe,
William Lee Irwin III
[-- Attachment #1: Type: text/plain, Size: 614 bytes --]
kunmap_atomic() violates the Principle of Least Surprise in a nasty way.
kmap(), kunmap(), and kmap_atomic() all take struct page* to
reference the memory location. kunmap_atomic() is the oddball of the
three, and takes a kernel address.
Ignoring the driver-related bugs that are present due to
kunmap_atomic()'s weirdness, there also appears to be a big in the
!CONFIG_HIGHMEM implementation in 2.4.x.
(Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
Anyway, what do people think about the attached patch to 2.4.x? I'm
surprised it has gone unnoticed until now.
Jeff
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 465 bytes --]
===== include/linux/highmem.h 1.12 vs edited =====
--- 1.12/include/linux/highmem.h 2003-06-30 20:18:42 -04:00
+++ edited/include/linux/highmem.h 2004-10-26 19:26:14 -04:00
@@ -70,7 +70,7 @@
#define kunmap(page) do { } while (0)
#define kmap_atomic(page,idx) kmap(page)
-#define kunmap_atomic(page,idx) kunmap(page)
+#define kunmap_atomic(addr,idx) kunmap(virt_to_page(addr))
#define bh_kmap(bh) ((bh)->b_data)
#define bh_kunmap(bh) do { } while (0)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.4] the perils of kunmap_atomic
2004-10-26 23:31 [PATCH 2.4] the perils of kunmap_atomic Jeff Garzik
@ 2004-10-26 23:36 ` Nigel Cunningham
2004-10-28 4:20 ` Jeff Garzik
2004-10-26 23:42 ` Nigel Cunningham
2004-10-27 16:19 ` Marcelo Tosatti
2 siblings, 1 reply; 7+ messages in thread
From: Nigel Cunningham @ 2004-10-26 23:36 UTC (permalink / raw)
To: Jeff Garzik
Cc: Linux Kernel Mailing List, Marcelo Tosatti,
Bartlomiej Zolnierkiewicz, Douglas Gilbert, Jens Axboe,
William Lee Irwin III
Hi.
On Wed, 2004-10-27 at 09:31, Jeff Garzik wrote:
> kunmap_atomic() violates the Principle of Least Surprise in a nasty way.
> kmap(), kunmap(), and kmap_atomic() all take struct page* to
> reference the memory location. kunmap_atomic() is the oddball of the
> three, and takes a kernel address.
>
> Ignoring the driver-related bugs that are present due to
> kunmap_atomic()'s weirdness, there also appears to be a big in the
> !CONFIG_HIGHMEM implementation in 2.4.x.
>
> (Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
>
> Anyway, what do people think about the attached patch to 2.4.x? I'm
> surprised it has gone unnoticed until now.
>
> Jeff
Ouch! It got me! That explains why suspend blows up with
CONFIG_DEBUG_HIGHMEM, but doesn't without it (2.6 - haven't tried
DEBUG_HIGHMEM under 2.4). It would be good if any patch produced a
warning if you call kunmap_atomic with the wrong kind of parameter.
Regards,
Nigel
>
> ______________________________________________________________________
> ===== include/linux/highmem.h 1.12 vs edited =====
> --- 1.12/include/linux/highmem.h 2003-06-30 20:18:42 -04:00
> +++ edited/include/linux/highmem.h 2004-10-26 19:26:14 -04:00
> @@ -70,7 +70,7 @@
> #define kunmap(page) do { } while (0)
>
> #define kmap_atomic(page,idx) kmap(page)
> -#define kunmap_atomic(page,idx) kunmap(page)
> +#define kunmap_atomic(addr,idx) kunmap(virt_to_page(addr))
>
> #define bh_kmap(bh) ((bh)->b_data)
> #define bh_kunmap(bh) do { } while (0)
--
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901
Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.4] the perils of kunmap_atomic
2004-10-26 23:31 [PATCH 2.4] the perils of kunmap_atomic Jeff Garzik
2004-10-26 23:36 ` Nigel Cunningham
@ 2004-10-26 23:42 ` Nigel Cunningham
2004-10-27 0:07 ` William Lee Irwin III
2004-10-27 16:19 ` Marcelo Tosatti
2 siblings, 1 reply; 7+ messages in thread
From: Nigel Cunningham @ 2004-10-26 23:42 UTC (permalink / raw)
To: Jeff Garzik
Cc: Linux Kernel Mailing List, Marcelo Tosatti,
Bartlomiej Zolnierkiewicz, Douglas Gilbert, Jens Axboe,
William Lee Irwin III
Hi again.
On Wed, 2004-10-27 at 09:31, Jeff Garzik wrote:
> kunmap_atomic() violates the Principle of Least Surprise in a nasty way.
> kmap(), kunmap(), and kmap_atomic() all take struct page* to
> reference the memory location. kunmap_atomic() is the oddball of the
> three, and takes a kernel address.
>
> Ignoring the driver-related bugs that are present due to
> kunmap_atomic()'s weirdness, there also appears to be a big in the
> !CONFIG_HIGHMEM implementation in 2.4.x.
>
> (Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
>
> Anyway, what do people think about the attached patch to 2.4.x? I'm
> surprised it has gone unnoticed until now.
>
> Jeff
On second thoughts, I think it's a bad idea to change the macro - in 2.6
at least. There are lots of uses of kunmap_atomic, and most of them do
the right thing. It's only inattentive people like me that need to fix
their code. :>
It would be good, though, to have kunmap_atomic warn on invalid
parameters (want a patch for that?)
Nigel
>
> ______________________________________________________________________
> ===== include/linux/highmem.h 1.12 vs edited =====
> --- 1.12/include/linux/highmem.h 2003-06-30 20:18:42 -04:00
> +++ edited/include/linux/highmem.h 2004-10-26 19:26:14 -04:00
> @@ -70,7 +70,7 @@
> #define kunmap(page) do { } while (0)
>
> #define kmap_atomic(page,idx) kmap(page)
> -#define kunmap_atomic(page,idx) kunmap(page)
> +#define kunmap_atomic(addr,idx) kunmap(virt_to_page(addr))
>
> #define bh_kmap(bh) ((bh)->b_data)
> #define bh_kunmap(bh) do { } while (0)
--
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901
Everyone lives by faith. Some people just don't believe it.
Want proof? Try to prove that the theory of evolution is true.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.4] the perils of kunmap_atomic
2004-10-26 23:42 ` Nigel Cunningham
@ 2004-10-27 0:07 ` William Lee Irwin III
0 siblings, 0 replies; 7+ messages in thread
From: William Lee Irwin III @ 2004-10-27 0:07 UTC (permalink / raw)
To: Nigel Cunningham
Cc: Jeff Garzik, Linux Kernel Mailing List, Marcelo Tosatti,
Bartlomiej Zolnierkiewicz, Douglas Gilbert, Jens Axboe
On Wed, 2004-10-27 at 09:31, Jeff Garzik wrote:
>> Ignoring the driver-related bugs that are present due to
>> kunmap_atomic()'s weirdness, there also appears to be a big in the
>> !CONFIG_HIGHMEM implementation in 2.4.x.
>> (Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
>> Anyway, what do people think about the attached patch to 2.4.x? I'm
>> surprised it has gone unnoticed until now.
On Wed, Oct 27, 2004 at 09:42:34AM +1000, Nigel Cunningham wrote:
> On second thoughts, I think it's a bad idea to change the macro - in 2.6
> at least. There are lots of uses of kunmap_atomic, and most of them do
> the right thing. It's only inattentive people like me that need to fix
> their code. :>
> It would be good, though, to have kunmap_atomic warn on invalid
> parameters (want a patch for that?)
The bug Jeff spotted is in 2.4.x only. It's probably worth spitting out
the expected and seen virtual address, and possibly the kmap index.
-- wli
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.4] the perils of kunmap_atomic
2004-10-26 23:31 [PATCH 2.4] the perils of kunmap_atomic Jeff Garzik
2004-10-26 23:36 ` Nigel Cunningham
2004-10-26 23:42 ` Nigel Cunningham
@ 2004-10-27 16:19 ` Marcelo Tosatti
2004-10-28 4:18 ` Jeff Garzik
2 siblings, 1 reply; 7+ messages in thread
From: Marcelo Tosatti @ 2004-10-27 16:19 UTC (permalink / raw)
To: Jeff Garzik
Cc: Linux Kernel, Bartlomiej Zolnierkiewicz, Douglas Gilbert,
Jens Axboe, William Lee Irwin III
On Tue, Oct 26, 2004 at 07:31:24PM -0400, Jeff Garzik wrote:
>
> kunmap_atomic() violates the Principle of Least Surprise in a nasty way.
> kmap(), kunmap(), and kmap_atomic() all take struct page* to
> reference the memory location. kunmap_atomic() is the oddball of the
> three, and takes a kernel address.
>
> Ignoring the driver-related bugs that are present due to
> kunmap_atomic()'s weirdness, there also appears to be a big in the
> !CONFIG_HIGHMEM implementation in 2.4.x.
>
> (Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
>
> Anyway, what do people think about the attached patch to 2.4.x? I'm
> surprised it has gone unnoticed until now.
>
> Jeff
>
> ===== include/linux/highmem.h 1.12 vs edited =====
> --- 1.12/include/linux/highmem.h 2003-06-30 20:18:42 -04:00
> +++ edited/include/linux/highmem.h 2004-10-26 19:26:14 -04:00
> @@ -70,7 +70,7 @@
> #define kunmap(page) do { } while (0)
>
> #define kmap_atomic(page,idx) kmap(page)
> -#define kunmap_atomic(page,idx) kunmap(page)
> +#define kunmap_atomic(addr,idx) kunmap(virt_to_page(addr))
>
> #define bh_kmap(bh) ((bh)->b_data)
> #define bh_kunmap(bh) do { } while (0)
Ugh :(
An audit of kunmap_atomic() users is needed.
We can try this in -29pre if there are no objections.
I have no useful comment about the bug itself right now.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.4] the perils of kunmap_atomic
2004-10-27 16:19 ` Marcelo Tosatti
@ 2004-10-28 4:18 ` Jeff Garzik
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Garzik @ 2004-10-28 4:18 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: Linux Kernel, Bartlomiej Zolnierkiewicz, Douglas Gilbert,
Jens Axboe, William Lee Irwin III
Marcelo Tosatti wrote:
> On Tue, Oct 26, 2004 at 07:31:24PM -0400, Jeff Garzik wrote:
>
>>kunmap_atomic() violates the Principle of Least Surprise in a nasty way.
>> kmap(), kunmap(), and kmap_atomic() all take struct page* to
>>reference the memory location. kunmap_atomic() is the oddball of the
>>three, and takes a kernel address.
>>
>>Ignoring the driver-related bugs that are present due to
>>kunmap_atomic()'s weirdness, there also appears to be a big in the
>>!CONFIG_HIGHMEM implementation in 2.4.x.
>>
>>(Bart is poking through some of the 2.6.x-related kunmap_atomic slip-ups)
>>
>>Anyway, what do people think about the attached patch to 2.4.x? I'm
>>surprised it has gone unnoticed until now.
>>
>> Jeff
>>
>>===== include/linux/highmem.h 1.12 vs edited =====
>>--- 1.12/include/linux/highmem.h 2003-06-30 20:18:42 -04:00
>>+++ edited/include/linux/highmem.h 2004-10-26 19:26:14 -04:00
>>@@ -70,7 +70,7 @@
>> #define kunmap(page) do { } while (0)
>>
>> #define kmap_atomic(page,idx) kmap(page)
>>-#define kunmap_atomic(page,idx) kunmap(page)
>>+#define kunmap_atomic(addr,idx) kunmap(virt_to_page(addr))
>>
>> #define bh_kmap(bh) ((bh)->b_data)
>> #define bh_kunmap(bh) do { } while (0)
>
>
> Ugh :(
Actually, a private email to me pointed out the obvious... kmap/kunmap
are no-ops on !CONFIG_HIGHMEM, so it's really a cosmetic bug, and my
patch won't fix anything but human confusion :)
> An audit of kunmap_atomic() users is needed.
agreed
Jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.4] the perils of kunmap_atomic
2004-10-26 23:36 ` Nigel Cunningham
@ 2004-10-28 4:20 ` Jeff Garzik
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Garzik @ 2004-10-28 4:20 UTC (permalink / raw)
To: ncunningham
Cc: Linux Kernel Mailing List, Marcelo Tosatti,
Bartlomiej Zolnierkiewicz, Douglas Gilbert, Jens Axboe,
William Lee Irwin III
Nigel Cunningham wrote:
> DEBUG_HIGHMEM under 2.4). It would be good if any patch produced a
> warning if you call kunmap_atomic with the wrong kind of parameter.
Well, the compiler has a rather difficult time with that, since any
kernel address is going to be void*, which C will nicely cast struct
page* into.
Jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2004-10-28 4:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-26 23:31 [PATCH 2.4] the perils of kunmap_atomic Jeff Garzik
2004-10-26 23:36 ` Nigel Cunningham
2004-10-28 4:20 ` Jeff Garzik
2004-10-26 23:42 ` Nigel Cunningham
2004-10-27 0:07 ` William Lee Irwin III
2004-10-27 16:19 ` Marcelo Tosatti
2004-10-28 4:18 ` Jeff Garzik
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).