Message ID | 20140428075149.GB28564@pengutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, Apr 28, 2014 at 09:51:49AM +0200, Uwe Kleine-König wrote: > On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote: > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > strncpy_from_user functions") apparently broken those string operations > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > restricts the addresses to TASK_SIZE. > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > restrict anything. > > > > Signed-off-by: Rabin Vincent <rabin@rab.in> > I tested this on my efm32 machine and it booted just fine. Before I used > a patch that did: > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 02fa2558f662..f25c7f4c5a44 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -92,9 +92,12 @@ > * It is difficult to define and perhaps will never meet the original meaning > * of this define that was meant to. > * Fortunately, there is no reference for this in noMMU mode, for now. > + * > + * HACK: copy_from_user must even handle copying from flash. So don't impose a > + * limit at all. Not sure this is correct ... > */ > #ifndef TASK_SIZE > -#define TASK_SIZE (CONFIG_DRAM_SIZE) > +#define TASK_SIZE (~0UL) > #endif The current code for user_addr_max() for !MMU is essentialy: #define user_addr_max() TASK_SIZE which is obviously wrong for the KERNEL_DS case, since it should be ~0UL. And user space can access all that the kernel does, so there should be no restriction for USER_DS either (which is anyway equivalent to KERNEL_DS). Hence, I think my patch, which removes the usage of TASK_SIZE in user_addr_max() for !MMU, is correct regardless of what the correct definition or meaning of TASK_SIZE for !MMU is. If you make TASK_SIZE to ~0UL (which is probably what it should be on !MMU), then the result is equivalent to my patch but it is not semantically correct since you are restricting user_addr_max() to TASK_SIZE even for the KERNEL_DS. What do you say?
Hello Rabin, On Mon, Jun 02, 2014 at 06:53:43PM +0200, Rabin Vincent wrote: > On Mon, Apr 28, 2014 at 09:51:49AM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote: > > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > > strncpy_from_user functions") apparently broken those string operations > > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > > restricts the addresses to TASK_SIZE. > > > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > > restrict anything. > > > > > > Signed-off-by: Rabin Vincent <rabin@rab.in> > > I tested this on my efm32 machine and it booted just fine. Before I used > > a patch that did: > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > index 02fa2558f662..f25c7f4c5a44 100644 > > --- a/arch/arm/include/asm/memory.h > > +++ b/arch/arm/include/asm/memory.h > > @@ -92,9 +92,12 @@ > > * It is difficult to define and perhaps will never meet the original meaning > > * of this define that was meant to. > > * Fortunately, there is no reference for this in noMMU mode, for now. > > + * > > + * HACK: copy_from_user must even handle copying from flash. So don't impose a > > + * limit at all. Not sure this is correct ... > > */ > > #ifndef TASK_SIZE > > -#define TASK_SIZE (CONFIG_DRAM_SIZE) > > +#define TASK_SIZE (~0UL) > > #endif > > The current code for user_addr_max() for !MMU is essentialy: > > #define user_addr_max() TASK_SIZE > > which is obviously wrong for the KERNEL_DS case, since it should be > ~0UL. And user space can access all that the kernel does, so there > should be no restriction for USER_DS either (which is anyway equivalent > to KERNEL_DS). Hence, I think my patch, which removes the usage of > TASK_SIZE in user_addr_max() for !MMU, is correct regardless of what the > correct definition or meaning of TASK_SIZE for !MMU is. > > If you make TASK_SIZE to ~0UL (which is probably what it should be on > !MMU), then the result is equivalent to my patch but it is not > semantically correct since you are restricting user_addr_max() to > TASK_SIZE even for the KERNEL_DS. I'd prefer to share as much code as possible between MMU and !MMU, so my preferred solution is: #ifndef CONFIG_MMU #define TASK_SIZE ~0UL /* do we need parentesis? */ #endif #define user_addr_max() \ (segment_eq(get_fs(), KERNEL_DS) ? ~0UL : TASK_SIZE) which should be correct and address your concern. Best regards Uwe
Hello, On Tue, Jun 03, 2014 at 09:51:33AM +0200, Uwe Kleine-König wrote: > On Mon, Jun 02, 2014 at 06:53:43PM +0200, Rabin Vincent wrote: > > On Mon, Apr 28, 2014 at 09:51:49AM +0200, Uwe Kleine-König wrote: > > > On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote: > > > > 8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and > > > > strncpy_from_user functions") apparently broken those string operations > > > > for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always > > > > restricts the addresses to TASK_SIZE. > > > > > > > > TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not > > > > restrict anything. > > > > > > > > Signed-off-by: Rabin Vincent <rabin@rab.in> > > > I tested this on my efm32 machine and it booted just fine. Before I used > > > a patch that did: > > > > > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > > > index 02fa2558f662..f25c7f4c5a44 100644 > > > --- a/arch/arm/include/asm/memory.h > > > +++ b/arch/arm/include/asm/memory.h > > > @@ -92,9 +92,12 @@ > > > * It is difficult to define and perhaps will never meet the original meaning > > > * of this define that was meant to. > > > * Fortunately, there is no reference for this in noMMU mode, for now. > > > + * > > > + * HACK: copy_from_user must even handle copying from flash. So don't impose a > > > + * limit at all. Not sure this is correct ... > > > */ > > > #ifndef TASK_SIZE > > > -#define TASK_SIZE (CONFIG_DRAM_SIZE) > > > +#define TASK_SIZE (~0UL) > > > #endif > > > > The current code for user_addr_max() for !MMU is essentialy: > > > > #define user_addr_max() TASK_SIZE > > > > which is obviously wrong for the KERNEL_DS case, since it should be > > ~0UL. And user space can access all that the kernel does, so there > > should be no restriction for USER_DS either (which is anyway equivalent > > to KERNEL_DS). Hence, I think my patch, which removes the usage of > > TASK_SIZE in user_addr_max() for !MMU, is correct regardless of what the > > correct definition or meaning of TASK_SIZE for !MMU is. > > > > If you make TASK_SIZE to ~0UL (which is probably what it should be on > > !MMU), then the result is equivalent to my patch but it is not > > semantically correct since you are restricting user_addr_max() to > > TASK_SIZE even for the KERNEL_DS. > I'd prefer to share as much code as possible between MMU and !MMU, so my > preferred solution is: > > #ifndef CONFIG_MMU > #define TASK_SIZE ~0UL /* do we need parentesis? */ > #endif > > #define user_addr_max() \ > (segment_eq(get_fs(), KERNEL_DS) ? ~0UL : TASK_SIZE) After looking into that a bit more I wonder if the correct version is (maybe an equivalent to): #define user_addr_max() \ (segment_eq(get_fs(), KERNEL_DS) ? ~0UL : get_fs()) That is because in the MMU case get_fs() is #defined as: #define get_fs() (current_thread_info()->addr_limit) and .addr_limit is changeable via set_fs. This would also mean that the current definition: #define user_addr_max() \ (segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL) might return ~0UL even though there is a limit which just happens not to be TASK_SIZE. (BTW, alpha, m68k, openrisc and sparc use the same definition.) Thoughts? Best regards Uwe
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h index 02fa2558f662..f25c7f4c5a44 100644 --- a/arch/arm/include/asm/memory.h +++ b/arch/arm/include/asm/memory.h @@ -92,9 +92,12 @@ * It is difficult to define and perhaps will never meet the original meaning * of this define that was meant to. * Fortunately, there is no reference for this in noMMU mode, for now. + * + * HACK: copy_from_user must even handle copying from flash. So don't impose a + * limit at all. Not sure this is correct ... */ #ifndef TASK_SIZE -#define TASK_SIZE (CONFIG_DRAM_SIZE) +#define TASK_SIZE (~0UL) #endif #ifndef TASK_UNMAPPED_BASE