Re: [PATCH 20/30] microblaze_mmu_v1: uaccess MMU update

[Arnd Bergmann <arnd@arndb.de>]

Following up on my earlier comment:

On Monday 27 April 2009, monstr@monstr.eu wrote:

> +#define __get_user(var, ptr)				\
> +({							\
> +	int __gu_err = 0;				\
> +	switch (sizeof(*(ptr))) {			\
> +	case 1:						\
> +	case 2:						\
> +	case 4:						\
> +		(var) = *(ptr);				\
> +		break;					\
> +	case 8:						\
> +		memcpy((void *) &(var), (ptr), 8);	\
> +		break;					\
> +	default:					\
> +		(var) = 0;				\
> +		__gu_err = __get_user_bad();		\
> +		break;					\
> +	}						\
> +	__gu_err;					\
> +})

This needs more __force to dereference the user pointers.
  
>  #define __get_user_bad()	(bad_user_access_length(), (-EFAULT))

You have actually defined bad_user_access_length(), which is not
how it __get_user_bad()	was meant as. The idea was to declare
an undefined function, which results in a link error in case
of funny length inputs to __get_user, a cheap way to do
BUILD_BUG_ON() in a switch() statement.

> +/* FIXME is not there defined __pu_val */
> +({								\
> +	int __pu_err = 0;					\
> +	switch (sizeof(*(ptr))) {				\
> +	case 1:							\
> +	case 2:							\
> +	case 4:							\
> +		*(ptr) = (var);					\
> +		break;						\
> +	case 8: {						\
> +		typeof(*(ptr)) __pu_val = (var);		\
> +		memcpy(ptr, &__pu_val, sizeof(__pu_val));	\
> +		}						\
> +		break;						\
> +	default:						\
> +		__pu_err = __put_user_bad();			\
> +		break;						\
> +	}							\
> +	__pu_err;						\
> +})
>  
>  #define __put_user_bad()	(bad_user_access_length(), (-EFAULT))

Same comments apply here.
  
> -#define put_user(x, ptr)	__put_user(x, ptr)
> -#define get_user(x, ptr)	__get_user(x, ptr)
> -
> -#define copy_to_user(to, from, n)		(memcpy(to, from, n), 0)
> -#define copy_from_user(to, from, n)		(memcpy(to, from, n), 0)
> +#define put_user(x, ptr)	__put_user((x), (ptr))
> +#define get_user(x, ptr)	__get_user((x), (ptr))

put_user and get_user should call access_ok() for the !MMU version I guess,
if you can easily detect kernel pointers based on the address.
It's also a good idea to add might_sleep() in there if access_ok()
doesn't have it already.

> -#define __copy_to_user(to, from, n)		(copy_to_user(to, from, n))
> -#define __copy_from_user(to, from, n)		(copy_from_user(to, from, n))
> -#define __copy_to_user_inatomic(to, from, n)	(__copy_to_user(to, from, n))
> -#define __copy_from_user_inatomic(to, from, n)	(__copy_from_user(to, from, n))
> +#define copy_to_user(to, from, n)	(memcpy((to), (from), (n)), 0)
> +#define copy_from_user(to, from, n)	(memcpy((to), (from), (n)), 0)

Same here, plus they can be shared between MMU and NOMMU.

> +#else /* CONFIG_MMU */
> +
> +/*
> + * Address is valid if:
> + *  - "addr", "addr + size" and "size" are all below the limit
> + */
> +#define access_ok(type, addr, size) \
> +	(get_fs().seg > (((unsigned long)(addr)) | \
> +		(size) | ((unsigned long)(addr) + (size))))
> +
> +/* || printk("access_ok failed for %s at 0x%08lx (size %d), seg 0x%08x\n",
> + type?"WRITE":"READ",addr,size,get_fs().seg)) */
> +
> +/*
> + * All the __XXX versions macros/functions below do not perform
> + * access checking. It is assumed that the necessary checks have been
> + * already performed before the finction (macro) is called.
> + */
> +
> +#define get_user(x, ptr)						\
> +({									\
> +	access_ok(VERIFY_READ, (ptr), sizeof(*(ptr)))			\
> +		? __get_user((x), (ptr)) : -EFAULT;			\
> +})
> +
> +#define put_user(x, ptr)						\
> +({									\
> +	access_ok(VERIFY_WRITE, (ptr), sizeof(*(ptr)))			\
> +		? __put_user((x), (ptr)) : -EFAULT;			\
> +})

Please add might_sleep() either here or in access_ok().

> +#define __get_user(x, ptr)						\
> +({									\
> +	unsigned long __gu_val;						\
> +	/*unsigned long __gu_ptr = (unsigned long)(ptr);*/		\
> +	long __gu_err;							\
> +	switch (sizeof(*(ptr))) {					\
> +	case 1:								\
> +		__get_user_asm("lbu", (ptr), __gu_val, __gu_err);	\
> +		break;							\
> +	case 2:								\
> +		__get_user_asm("lhu", (ptr), __gu_val, __gu_err);	\
> +		break;							\
> +	case 4:								\
> +		__get_user_asm("lw", (ptr), __gu_val, __gu_err);	\
> +		break;							\
> +	default:							\
> +		__gu_val = 0; __gu_err = -EINVAL;			\
> +	}								\
> +	x = (__typeof__(*(ptr))) __gu_val;				\
> +	__gu_err;							\
> +})

Again, the 'default:' case should give a link error, not a runtime
error. 

It seems inconsistent to have a 'case 8:' in !MMU as well as both
__put_user variants but not in the MMU __get_user.

> +#define __put_user(x, ptr)						\
> +({									\
> +	__typeof__(*(ptr)) __gu_val = x;				\
> +	long __gu_err = 0;						\
> +	switch (sizeof(__gu_val)) {					\
> +	case 1:								\
> +		__put_user_asm("sb", (ptr), __gu_val, __gu_err);	\
> +		break;							\
> +	case 2: 							\
> +		__put_user_asm("sh", (ptr), __gu_val, __gu_err);	\
> +		break;							\
> +	case 4:								\
> +		__put_user_asm("sw", (ptr), __gu_val, __gu_err);	\
> +		break;							\
> +	case 8:								\
> +		__put_user_asm_8((ptr), __gu_val, __gu_err);		\
> +		break;							\
> +	default:							\
> +		__gu_err = -EINVAL;					\
> +	}								\
> +	__gu_err;							\
> +})

> +/*
> + * Return: number of not copied bytes, i.e. 0 if OK or non-zero if fail.
> + */
> +static inline int clear_user(char *to, int size)
> +{
> +	if (size && access_ok(VERIFY_WRITE, to, size)) {
> +		__asm__ __volatile__ ("				\
> +				1:				\
> +					sb	r0, %2, r0;	\
> +					addik	%0, %0, -1;	\
> +					bneid	%0, 1b;		\
> +					addik	%2, %2, 1;	\
> +				2:				\
> +				.section __ex_table,\"a\";	\
> +				.word	1b,2b;			\
> +				.section .text;"		\
> +			: "=r"(size)				\
> +			: "0"(size), "r"(to)
> +		);
> +	}
> +	return size;
> +}

Please use the prototype I mentioned in the other mail.

While I don't remember the exact story, I think the preferred
way to write multi-line inline assembly is

	asm volatile("# clear_user		\n"
		"1:				\n"
		"	sb r0, %2, r0		\n"
		"	addik	%0, %0, -1	\n"
		"	bneid	%0, 1b		\n"
		"2:				\n"
		".section __ex_table,\"a\"	\n"
		".word	1b,2b;			\n"
		".section .text;		\n");

rather than a single multi-line string.

> +extern unsigned long __copy_tofrom_user(void __user *to,
> +		const void __user *from, unsigned long size);
> +
> +#define copy_to_user(to, from, n)					\
> +	(access_ok(VERIFY_WRITE, (to), (n)) ?				\
> +		__copy_tofrom_user((void __user *)(to),			\
> +			(__force const void __user *)(from), (n))	\
> +		: -EFAULT)

No, copy_to_user does *not* return an errno, but instead the number
of bytes not copied. Assuming your __copy_tofrom_user is correct,
this would be

static inline __must_check unsigned long
copy_to_user(void __user *to, void *from, size_t n)
{
	might_sleep();

	if (!access_ok(VERIFY_WRITE, to, n))
		return n;

	return __copy_tofrom_user(to, (const void __force __user *)from, n);
}

> +#define __copy_to_user(to, from, n)	copy_to_user((to), (from), (n))
> +#define __copy_to_user_inatomic(to, from, n)	copy_to_user((to), (from), (n))
> +
> +#define copy_from_user(to, from, n)					\
> +	(access_ok(VERIFY_READ, (from), (n)) ?				\
> +		__copy_tofrom_user((__force void __user *)(to),		\
> +			(void __user *)(from), (n))			\
> +		: -EFAULT)

same here

> +#define __copy_from_user(to, from, n)	copy_from_user((to), (from), (n))
> +#define __copy_from_user_inatomic(to, from, n) \
> +		copy_from_user((to), (from), (n))
> +
> +extern int __strncpy_user(char *to, const char __user *from, int len);
> +extern int __strnlen_user(const char __user *sstr, int len);
> +
> +#define strncpy_from_user(to, from, len)	\
> +		(access_ok(VERIFY_READ, from, 1) ?	\
> +			__strncpy_user(to, from, len) : -EFAULT)

and here.

	Arnd <><