| Message ID | 20111202222623.GA31354@us.ibm.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
> +static inline int page_is_rtas_user_buf(unsigned long pfn) > +{ > + unsigned long paddr = (pfn << PAGE_SHIFT); > + if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + > RTAS_RMOBUF_MAX)) It probably cannot overflow with actual values of rtas_rmo_buf and RTAS_RMOBUF_MAX, but otherwise it is an incorrect test; please write if (paddr >= rtas_rmo_buf && paddr - rtas_rmo_buf < RTAS_RMOBUF_MAX) (and, _MAX? Shouldn't it be the actual size here? Or is _MAX just a confusing name :-) ) Segher
On Sat, 2011-12-03 at 04:22 +0100, Segher Boessenkool wrote: > > +static inline int page_is_rtas_user_buf(unsigned long pfn) > > +{ > > + unsigned long paddr = (pfn << PAGE_SHIFT); > > + if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + > > RTAS_RMOBUF_MAX)) > > It probably cannot overflow with actual values of rtas_rmo_buf > and RTAS_RMOBUF_MAX, but otherwise it is an incorrect test; > please write > > if (paddr >= rtas_rmo_buf && paddr - rtas_rmo_buf < RTAS_RMOBUF_MAX) > > (and, _MAX? Shouldn't it be the actual size here? Or is _MAX > just a confusing name :-) ) The original code is a lot more readable and perfectly correct for all possible values of rtas_rmo_buf :-) Cheers, Ben.
>>> +static inline int page_is_rtas_user_buf(unsigned long pfn) >>> +{ >>> + unsigned long paddr = (pfn << PAGE_SHIFT); >>> + if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + >>> RTAS_RMOBUF_MAX)) >> >> It probably cannot overflow with actual values of rtas_rmo_buf >> and RTAS_RMOBUF_MAX, but otherwise it is an incorrect test; >> please write >> >> if (paddr >= rtas_rmo_buf && paddr - rtas_rmo_buf < RTAS_RMOBUF_MAX) >> >> (and, _MAX? Shouldn't it be the actual size here? Or is _MAX >> just a confusing name :-) ) > > The original code is a lot more readable and perfectly correct for all > possible values of rtas_rmo_buf :-) You have to consider those possible values before you see it cannot overflow. So no, it's a lot _less_ readable IMHO. It's also a dangerous habit to get into. Just say no :-) Segher
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h index 58625d1..c2d3c9f 100644 --- a/arch/powerpc/include/asm/rtas.h +++ b/arch/powerpc/include/asm/rtas.h @@ -305,5 +305,17 @@ static inline u32 rtas_config_addr(int busno, int devfn, int reg) extern void __cpuinit rtas_give_timebase(void); extern void __cpuinit rtas_take_timebase(void); +#ifdef CONFIG_PPC_RTAS +static inline int page_is_rtas_user_buf(unsigned long pfn) +{ + unsigned long paddr = (pfn << PAGE_SHIFT); + if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + RTAS_RMOBUF_MAX)) + return 1; + return 0; +} +#else +static inline int page_is_rtas_user_buf(unsigned long pfn) { return 0;} +#endif + #endif /* __KERNEL__ */ #endif /* _POWERPC_RTAS_H */ diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index 553fb41..05abd49 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -50,6 +50,7 @@ #include <asm/vdso.h> #include <asm/fixmap.h> #include <asm/swiotlb.h> +#include <asm/rtas.h> #include "mmu_decl.h" @@ -564,6 +565,8 @@ int devmem_is_allowed(unsigned long pfn) return 0; if (!page_is_ram(pfn)) return 1; + if (page_is_rtas_user_buf(pfn)) + return 1; return 0; } #endif /* CONFIG_STRICT_DEVMEM */
Subject: [PATCH 1/1] Punch a hole in /dev/mem for librtas With CONFIG_STRICT_DEVMEM=y, user space cannot read any part of /dev/mem. Since this breaks librtas, punch a hole in /dev/mem to allow access to the rmo_buffer that librtas needs. Anton Blanchard reported the problem and helped with the fix. A quick test for this patch: # cat /proc/rtas/rmo_buffer 000000000f190000 10000 # python -c "print 0x000000000f190000 / 0x10000" 3865 # dd if=/dev/mem of=/tmp/foo count=1 bs=64k skip=3865 1+0 records in 1+0 records out 65536 bytes (66 kB) copied, 0.000205235 s, 319 MB/s # dd if=/dev/mem of=/tmp/foo dd: reading `/dev/mem': Operation not permitted 0+0 records in 0+0 records out 0 bytes (0 B) copied, 0.00022519 s, 0.0 kB/s Changelog[v3]: [Ben Harrenschmidt]: Incremental patch for the punched hole, since CONFIG_STRICT_DEVMEM was merged into the -next branch. [Ben Harrenschmidt]: Rename interface to page_is_rtas_user_buf() move declaration to a header file and ensure it doesn't break CONFIG_PPC_RTAS=n. Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> --- arch/powerpc/include/asm/rtas.h | 12 ++++++++++++ arch/powerpc/mm/mem.c | 3 +++ 2 files changed, 15 insertions(+), 0 deletions(-)