[PATCH v4 06/25] time/posix-timers:Introduce {get,put}_timespec and {get,put}_itimerspec

[PATCH v4 06/25] time/posix-timers:Introduce {get,put}_timespec and {get,put}_itimerspec

[Baolin Wang]

These are new helper functions that convert between a user timespec/
itimerspec and a kernel timespec64/itimerspec64 structure.

These macros can change the types underneath from both ends and it
will work efficiently on both 32-bit and 64-bit that can avoid the
CONFIG_64BIT macro in syscall functions, and also it can make the
syscall functions more simple.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 kernel/time/posix-timers.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 31ea01f..96efe1d 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -147,6 +147,35 @@ static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags);
 	__timr;								   \
 })
 
+#define __get_timespec(kts, uts) \
+	(__get_user((kts)->tv_sec, &(uts)->tv_sec) || \
+	__get_user((kts)->tv_nsec, &(uts)->tv_nsec))
+
+#define __put_timespec(kts, uts) \
+	(__put_user((kts)->tv_sec, &(uts)->tv_sec) || \
+	__put_user((kts)->tv_nsec, &(uts)->tv_nsec))
+
+#define get_timespec(kts, uts) \
+	((access_ok(VERIFY_READ, (uts), sizeof(*(uts))) || \
+	__get_timespec((kts), (uts))) ? \
+	-EFAULT : 0)
+
+#define put_timespec(kts, uts) \
+	((access_ok(VERIFY_WRITE, (uts), sizeof(*(uts))) || \
+	__put_timespec((kts), (uts))) ? \
+	-EFAULT : 0)
+
+#define get_itimerspec(kit, uit) \
+	((access_ok(VERIFY_READ, (uit), sizeof(*(uit))) || \
+	__get_timespec(&(kit)->it_interval, &(uit)->it_interval) || \
+	__get_timespec(&(kit)->it_value, &(uit)->it_value)))
+
+#define put_itimerspec(kit, uit) \
+	((access_ok(VERIFY_WRITE, (uit), sizeof(*(uit))) || \
+	__put_timespec(&(kit)->it_interval, &(uit)->it_interval) || \
+	__put_timespec(&(kit)->it_value, &(uit)->it_value)) ? \
+	-EFAULT : 0)
+
 static int hash(struct signal_struct *sig, unsigned int nr)
 {
 	return hash_32(hash32_ptr(sig) ^ nr, HASH_BITS(posix_timers_hashtable));
-- 
1.7.9.5

Re: [PATCH v4 06/25] time/posix-timers:Introduce {get,put}_timespec and {get,put}_itimerspec

[Thomas Gleixner]

On Mon, 1 Jun 2015, Baolin Wang wrote:

> These are new helper functions that convert between a user timespec/
> itimerspec and a kernel timespec64/itimerspec64 structure.

These are not functions, these are macros.
 
> These macros can change the types underneath from both ends and it
> will work efficiently on both 32-bit and 64-bit that can avoid the
> CONFIG_64BIT macro in syscall functions, and also it can make the
> syscall functions more simple.

Lots of useless blurb which fails to explain WHY this works and WHY
this magically converts the types.

And you fail to mention WHY dropping type safety is a good choice and
WHY dropping the might_fault() check is a proper thing to do.

I also doubt the efficiency part as you replace a linear
copy_to_user() with four seperate ones for an itimerspec.
  
This can be done proper with typesafe inline helpers, if you want to
spare the ifdef in the syscall implementation.

Thanks,

	tglx

Re: [Y2038] [PATCH v4 06/25] time/posix-timers:Introduce {get, put}_timespec and {get, put}_itimerspec

[Arnd Bergmann]

On Tuesday 02 June 2015 21:20:08 Thomas Gleixner wrote:
> On Mon, 1 Jun 2015, Baolin Wang wrote:
> 
> > These are new helper functions that convert between a user timespec/
> > itimerspec and a kernel timespec64/itimerspec64 structure.
> 
> These are not functions, these are macros.
>  
> > These macros can change the types underneath from both ends and it
> > will work efficiently on both 32-bit and 64-bit that can avoid the
> > CONFIG_64BIT macro in syscall functions, and also it can make the
> > syscall functions more simple.
> 
> Lots of useless blurb which fails to explain WHY this works and WHY
> this magically converts the types.
> 
> And you fail to mention WHY dropping type safety is a good choice and
> WHY dropping the might_fault() check is a proper thing to do.
> 
> I also doubt the efficiency part as you replace a linear
> copy_to_user() with four seperate ones for an itimerspec.
>   
> This can be done proper with typesafe inline helpers, if you want to
> spare the ifdef in the syscall implementation.
> 

I suggested these macros on IRC, as a way to help coordinate Baolin's
series with my own patches that conver the entry points at first to
use __kernel_timespec equal to the normal timespec, and then changing
that type to be based on __kernel_time64_t.

Specifically, we otherwise need to deal with these combinations:

user timespec (32 bit), kernel timespec (32 bit)
user timespec (64 bit), kernel timespec (64 bit)
user timespec (32 bit), kernel timespec64 (64 bit)
user timespec (64 bit), kernel timespec64 (64 bit)
user __kernel_timespec (32 bit), kernel timespec (32 bit)
user __kernel_timespec (64 bit), kernel timespec (32 bit)
user __kernel_timespec (64 bit), kernel timespec (64 bit)
user __kernel_timespec (32 bit), kernel timespec64 (64 bit)
user __kernel_timespec (64 bit), kernel timespec64 (64 bit)

My existing patche series handles this with fully type-safe functions,
but causes more churn than using less safe functions, which can
handle all the combinations above.

We could also do untyped get/put functions based on copy_to_user
and copy_from_user, but I guess what you're after is more along the
lines of typed accessor functions like I had at first:

int get_timespec64(struct timespec64 *ts, const struct timespec __user *uts)
{
	struct timespec64 tmp;
	int ret;

	if (sizeof(tmp) == sizeof(*ts))
		return copy_from_user(&tmp, uts, sizeof(*ts)) ? -EFAULT : 0;

	ret = copy_from_user(&tmp, uts, sizeof(*ts));
	if (ret)
		return -EFAULT;

	ts->tv_sec = tmp.tv_sec;
	ts->tv_nsec = tmp.tv_nsec;

	return 0;
}

This works fine, but I'd have to change it to copy from a __user
__kernel_timespec instead of timespec in my system call series, and
in order to do that, we must ensure that I can change over all callers
at the same time, so with the function prototype above, we should not
start using get_timespec64 for anything outside of posix-timers.c.

	Arnd

Re: [Y2038] [PATCH v4 06/25] time/posix-timers:Introduce {get, put}_timespec and {get, put}_itimerspec

[Thomas Gleixner]

On Thu, 4 Jun 2015, Arnd Bergmann wrote:
> int get_timespec64(struct timespec64 *ts, const struct timespec __user *uts)
> {
> 	struct timespec64 tmp;
> 	int ret;
> 
> 	if (sizeof(tmp) == sizeof(*ts))
> 		return copy_from_user(&tmp, uts, sizeof(*ts)) ? -EFAULT : 0;
> 
> 	ret = copy_from_user(&tmp, uts, sizeof(*ts));
> 	if (ret)
> 		return -EFAULT;
> 
> 	ts->tv_sec = tmp.tv_sec;
> 	ts->tv_nsec = tmp.tv_nsec;
> 
> 	return 0;
> }
> 
> This works fine, but I'd have to change it to copy from a __user
> __kernel_timespec instead of timespec in my system call series, and
> in order to do that, we must ensure that I can change over all callers
> at the same time, so with the function prototype above, we should not
> start using get_timespec64 for anything outside of posix-timers.c.

That's fine I think.

Thanks,

	tglx