the perils of kunmap_atomic
PATCH 1/1 the perils of kunmap_atomic
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 =====
include/linux/highmem.h +1 -1
@@ -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)
Hi.
Quoted 9 lines · Jeff Garzik
kunmap_atomic()violates the Principle of Least Surprise in a nasty way.
kmap(),kunmap(), andkmap_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.
Quoted 6 lines
(Bart is poking through some of the 2.6.x-related
kunmap_atomicslip-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
Quoted 14 lines
______________________________________________________________________
=====include/linux/highmem.h1.12 vs edited =====include/linux/highmem.h +1
-1@@ -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)
Signature · 7 lines
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.
Hi again.
Quoted 9 lines · Jeff Garzik
kunmap_atomic()violates the Principle of Least Surprise in a nasty way.
kmap(),kunmap(), andkmap_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.
Quoted 6 lines
(Bart is poking through some of the 2.6.x-related
kunmap_atomicslip-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
Quoted 14 lines
______________________________________________________________________
=====include/linux/highmem.h1.12 vs edited =====include/linux/highmem.h +1
-1@@ -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)
Signature · 7 lines
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.
Quoted 6 lines · Jeff Garzik
Quoted 6 lines
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-relatedkunmap_atomicslip-ups) Anyway, what do people think about the attached patch to 2.4.x? I'm surprised it has gone unnoticed until now.
Quoted 6 lines · Nigel Cunningham
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 havekunmap_atomicwarn 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
Quoted 21 lines · Jeff Garzik
kunmap_atomic()violates the Principle of Least Surprise in a nasty way.
kmap(),kunmap(), andkmap_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_atomicslip-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.h1.12 vs edited =====
include/linux/highmem.h line 76
Quoted 8 lines
include/linux/highmem.h +1
-1@@ -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.
Quoted 24 lines · Marcelo Tosatti
Quoted 22 lines · Jeff Garzik
kunmap_atomic()violates the Principle of Least Surprise in a nasty way.
kmap(),kunmap(), andkmap_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_atomicslip-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.h1.12 vs edited =====include/linux/highmem.h +1
-1@@ -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))
Quoted 5 lines
include/linux/highmem.h line 76
Quoted 2 lines
include/linux/highmem.h +0
-0#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 :)
Quoted 1 line
An audit of
kunmap_atomic()users is needed.
agreed
Jeff
Quoted 2 lines · Nigel Cunningham
DEBUG_HIGHMEMunder 2.4). It would be good if any patch produced a warning if you callkunmap_atomicwith 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