[PATCH 1/1] aio: make sure the input "timeout" value is valid

[PATCH 1/1] aio: make sure the input "timeout" value is valid

[Zhen Lei]

Below information is reported by a lower kernel version, and I saw the
problem still exist in current version.

UBSAN: Undefined behaviour in include/linux/ktime.h:55:34
signed integer overflow:
-4971973988617027584 * 1000000000 cannot be represented in type 'long int'
......
[<ffff80000072ca28>] timespec_to_ktime include/linux/ktime.h:55 [inline]
[<ffff80000072ca28>] read_events+0x4c8/0x5d0 fs/aio.c:1269
[<ffff8000007305bc>] SYSC_io_getevents fs/aio.c:1733 [inline]
[<ffff8000007305bc>] SyS_io_getevents+0xd4/0x218 fs/aio.c:1722

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 fs/aio.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/aio.c b/fs/aio.c
index a062d75..19f7661 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1858,6 +1858,9 @@ static long do_io_getevents(aio_context_t ctx_id,
 	if (timeout) {
 		if (unlikely(get_timespec64(&ts, timeout)))
 			return -EFAULT;
+
+		if (!timespec64_valid(&ts))
+			return -EINVAL;
 	}

 	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
@@ -1876,6 +1879,8 @@ static long do_io_getevents(aio_context_t ctx_id,
 		if (compat_get_timespec64(&t, timeout))
 			return -EFAULT;

+		if (!timespec64_valid(&t))
+			return -EINVAL;
 	}

 	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
--
1.8.3

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Matthew Wilcox]

On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
> Below information is reported by a lower kernel version, and I saw the
> problem still exist in current version.

I think you're right, but what an awful interface we have here!
The user must not only fetch it, they must validate it separately?
And if they forget, then userspace is provoking undefined behaviour?  Ugh.
Why not this:

diff --git a/fs/aio.c b/fs/aio.c
index 4adbdcbe753a..fdd16cf897c8 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1788,8 +1788,9 @@ SYSCALL_DEFINE5(io_getevents, aio_context_t, ctx_id,
 	struct timespec64	ts;
 
 	if (timeout) {
-		if (unlikely(get_timespec64(&ts, timeout)))
-			return -EFAULT;
+		int error = get_valid_timespec64(&ts, timeout);
+		if (error)
+			return error;
 	}
 
 	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
@@ -1805,9 +1806,9 @@ COMPAT_SYSCALL_DEFINE5(io_getevents, compat_aio_context_t, ctx_id,
 	struct timespec64 t;
 
 	if (timeout) {
-		if (compat_get_timespec64(&t, timeout))
-			return -EFAULT;
-
+		int error = compat_get_valid_timespec64(&t, timeout);
+		if (error)
+			return error;
 	}
 
 	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 0fc36406f32c..578fc0f208d9 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -172,6 +172,26 @@ extern int get_compat_itimerspec64(struct itimerspec64 *its,
 extern int put_compat_itimerspec64(const struct itimerspec64 *its,
 			struct compat_itimerspec __user *uits);
 
+static inline __must_check
+int compat_get_valid_timespec64(struct timespec64 *ts, const void __user *uts)
+{
+	if (unlikely(compat_get_timespec64(ts, uts)))
+		return -EFAULT;
+	if (unlikely(!timespec64_valid(ts)))
+		return -EINVAL;
+	return 0;
+}
+
+static inline __must_check
+int compat_get_strict_timespec64(struct timespec64 *ts, const void __user *uts)
+{
+	if (unlikely(compat_get_timespec64(ts, uts)))
+		return -EFAULT;
+	if (unlikely(!timespec64_valid_strict(ts)))
+		return -EINVAL;
+	return 0;
+}
+
 struct compat_iovec {
 	compat_uptr_t	iov_base;
 	compat_size_t	iov_len;
diff --git a/include/linux/time.h b/include/linux/time.h
index 4b62a2c0a661..506d87483d04 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -18,6 +18,26 @@ int get_itimerspec64(struct itimerspec64 *it,
 int put_itimerspec64(const struct itimerspec64 *it,
 			struct itimerspec __user *uit);
 
+static inline __must_check int get_valid_timespec64(struct timespec64 *ts,
+					const struct timespec __user *uts)
+{
+	if (unlikely(get_timespec64(ts, uts)))
+		return -EFAULT;
+	if (unlikely(!timespec64_valid(ts)))
+		return -EINVAL;
+	return 0;
+}
+
+static inline __must_check int get_strict_timespec64(struct timespec64 *ts,
+					const struct timespec __user *uts)
+{
+	if (unlikely(get_timespec64(ts, uts)))
+		return -EFAULT;
+	if (unlikely(!timespec64_valid_strict(ts)))
+		return -EINVAL;
+	return 0;
+}
+
 extern time64_t mktime64(const unsigned int year, const unsigned int mon,
 			const unsigned int day, const unsigned int hour,
 			const unsigned int min, const unsigned int sec);

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Benjamin LaHaise]

On Wed, Dec 13, 2017 at 06:11:12AM -0800, Matthew Wilcox wrote:
> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
> > Below information is reported by a lower kernel version, and I saw the
> > problem still exist in current version.
> 
> I think you're right, but what an awful interface we have here!
> The user must not only fetch it, they must validate it separately?
> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
> Why not this:

That looks far better!

Acked-by: Benjamin LaHaise <bcrl@kvack.org>

		-ben

> diff --git a/fs/aio.c b/fs/aio.c
> index 4adbdcbe753a..fdd16cf897c8 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1788,8 +1788,9 @@ SYSCALL_DEFINE5(io_getevents, aio_context_t, ctx_id,
>  	struct timespec64	ts;
>  
>  	if (timeout) {
> -		if (unlikely(get_timespec64(&ts, timeout)))
> -			return -EFAULT;
> +		int error = get_valid_timespec64(&ts, timeout);
> +		if (error)
> +			return error;
>  	}
>  
>  	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
> @@ -1805,9 +1806,9 @@ COMPAT_SYSCALL_DEFINE5(io_getevents, compat_aio_context_t, ctx_id,
>  	struct timespec64 t;
>  
>  	if (timeout) {
> -		if (compat_get_timespec64(&t, timeout))
> -			return -EFAULT;
> -
> +		int error = compat_get_valid_timespec64(&t, timeout);
> +		if (error)
> +			return error;
>  	}
>  
>  	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 0fc36406f32c..578fc0f208d9 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -172,6 +172,26 @@ extern int get_compat_itimerspec64(struct itimerspec64 *its,
>  extern int put_compat_itimerspec64(const struct itimerspec64 *its,
>  			struct compat_itimerspec __user *uits);
>  
> +static inline __must_check
> +int compat_get_valid_timespec64(struct timespec64 *ts, const void __user *uts)
> +{
> +	if (unlikely(compat_get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static inline __must_check
> +int compat_get_strict_timespec64(struct timespec64 *ts, const void __user *uts)
> +{
> +	if (unlikely(compat_get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid_strict(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  struct compat_iovec {
>  	compat_uptr_t	iov_base;
>  	compat_size_t	iov_len;
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 4b62a2c0a661..506d87483d04 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -18,6 +18,26 @@ int get_itimerspec64(struct itimerspec64 *it,
>  int put_itimerspec64(const struct itimerspec64 *it,
>  			struct itimerspec __user *uit);
>  
> +static inline __must_check int get_valid_timespec64(struct timespec64 *ts,
> +					const struct timespec __user *uts)
> +{
> +	if (unlikely(get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static inline __must_check int get_strict_timespec64(struct timespec64 *ts,
> +					const struct timespec __user *uts)
> +{
> +	if (unlikely(get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid_strict(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  extern time64_t mktime64(const unsigned int year, const unsigned int mon,
>  			const unsigned int day, const unsigned int hour,
>  			const unsigned int min, const unsigned int sec);
> 

-- 
"Thought is the essence of where you are now."

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Jeff Moyer]

Matthew Wilcox <willy@infradead.org> writes:

> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
>> Below information is reported by a lower kernel version, and I saw the
>> problem still exist in current version.
>
> I think you're right, but what an awful interface we have here!
> The user must not only fetch it, they must validate it separately?
> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
> Why not this:

Why not go a step further and have get_timespec64 check for validity?
I wonder what caller doesn't want that to happen...

-Jeff

>
> diff --git a/fs/aio.c b/fs/aio.c
> index 4adbdcbe753a..fdd16cf897c8 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1788,8 +1788,9 @@ SYSCALL_DEFINE5(io_getevents, aio_context_t, ctx_id,
>  	struct timespec64	ts;
>  
>  	if (timeout) {
> -		if (unlikely(get_timespec64(&ts, timeout)))
> -			return -EFAULT;
> +		int error = get_valid_timespec64(&ts, timeout);
> +		if (error)
> +			return error;
>  	}
>  
>  	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
> @@ -1805,9 +1806,9 @@ COMPAT_SYSCALL_DEFINE5(io_getevents, compat_aio_context_t, ctx_id,
>  	struct timespec64 t;
>  
>  	if (timeout) {
> -		if (compat_get_timespec64(&t, timeout))
> -			return -EFAULT;
> -
> +		int error = compat_get_valid_timespec64(&t, timeout);
> +		if (error)
> +			return error;
>  	}
>  
>  	return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 0fc36406f32c..578fc0f208d9 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -172,6 +172,26 @@ extern int get_compat_itimerspec64(struct itimerspec64 *its,
>  extern int put_compat_itimerspec64(const struct itimerspec64 *its,
>  			struct compat_itimerspec __user *uits);
>  
> +static inline __must_check
> +int compat_get_valid_timespec64(struct timespec64 *ts, const void __user *uts)
> +{
> +	if (unlikely(compat_get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static inline __must_check
> +int compat_get_strict_timespec64(struct timespec64 *ts, const void __user *uts)
> +{
> +	if (unlikely(compat_get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid_strict(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  struct compat_iovec {
>  	compat_uptr_t	iov_base;
>  	compat_size_t	iov_len;
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 4b62a2c0a661..506d87483d04 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -18,6 +18,26 @@ int get_itimerspec64(struct itimerspec64 *it,
>  int put_itimerspec64(const struct itimerspec64 *it,
>  			struct itimerspec __user *uit);
>  
> +static inline __must_check int get_valid_timespec64(struct timespec64 *ts,
> +					const struct timespec __user *uts)
> +{
> +	if (unlikely(get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static inline __must_check int get_strict_timespec64(struct timespec64 *ts,
> +					const struct timespec __user *uts)
> +{
> +	if (unlikely(get_timespec64(ts, uts)))
> +		return -EFAULT;
> +	if (unlikely(!timespec64_valid_strict(ts)))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  extern time64_t mktime64(const unsigned int year, const unsigned int mon,
>  			const unsigned int day, const unsigned int hour,
>  			const unsigned int min, const unsigned int sec);
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-aio' in
> the body to majordomo@kvack.org.  For more info on Linux AIO,
> see: http://www.kvack.org/aio/
> Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Matthew Wilcox]

On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
> >> Below information is reported by a lower kernel version, and I saw the
> >> problem still exist in current version.
> >
> > I think you're right, but what an awful interface we have here!
> > The user must not only fetch it, they must validate it separately?
> > And if they forget, then userspace is provoking undefined behaviour?  Ugh.
> > Why not this:
> 
> Why not go a step further and have get_timespec64 check for validity?
> I wonder what caller doesn't want that to happen...

There are some which don't today.  I'm hoping Deepa takes this and goes
off and fixes them all up.

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Leizhen (ThunderTown)]

On 2017/12/14 3:31, Matthew Wilcox wrote:
> On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
>> Matthew Wilcox <willy@infradead.org> writes:
>>
>>> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
>>>> Below information is reported by a lower kernel version, and I saw the
>>>> problem still exist in current version.
>>>
>>> I think you're right, but what an awful interface we have here!
>>> The user must not only fetch it, they must validate it separately?
>>> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
>>> Why not this:
>>
>> Why not go a step further and have get_timespec64 check for validity?
>> I wonder what caller doesn't want that to happen...
I tried this before. But I found some places call get_timespec64 in the following function.
If we do the check in get_timespec64, the check will be duplicated.

For example:
static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
....
	if (get_timespec64(&ts, tsp))
		return -EFAULT;

	to = &end_time;
	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))

int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
{
	struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};

	if (!timespec64_valid(&ts))
		return -EINVAL;

> 
> There are some which don't today.  I'm hoping Deepa takes this and goes
> off and fixes them all up.
As my search results, just the case I mentioned above, which may cause duplicate check.
So if we don't care the slightly performance drop, maybe we should do timespec64_valid
check in get_timespec64. I can try this in v2. Otherwise, use your method.

> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Matthew Wilcox]

On Thu, Dec 14, 2017 at 11:18:30AM +0800, Leizhen (ThunderTown) wrote:
> On 2017/12/14 3:31, Matthew Wilcox wrote:
> > On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
> >> Matthew Wilcox <willy@infradead.org> writes:
> >>
> >>> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
> >>>> Below information is reported by a lower kernel version, and I saw the
> >>>> problem still exist in current version.
> >>>
> >>> I think you're right, but what an awful interface we have here!
> >>> The user must not only fetch it, they must validate it separately?
> >>> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
> >>> Why not this:
> >>
> >> Why not go a step further and have get_timespec64 check for validity?
> >> I wonder what caller doesn't want that to happen...
> I tried this before. But I found some places call get_timespec64 in the following function.
> If we do the check in get_timespec64, the check will be duplicated.
> 
> For example:
> static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
> ....
> 	if (get_timespec64(&ts, tsp))
> 		return -EFAULT;
> 
> 	to = &end_time;
> 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
> 
> int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
> {
> 	struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};
> 
> 	if (!timespec64_valid(&ts))
> 		return -EINVAL;

The check is only two comparisons!  Why do we have an interface that can
cause bugs for the sake of saving *two comparisons*?!  Can we talk about
the cost of a cache miss versus the cost of executing these comparisons?

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Jeff Moyer]

Matthew Wilcox <willy@infradead.org> writes:

> On Thu, Dec 14, 2017 at 11:18:30AM +0800, Leizhen (ThunderTown) wrote:
>> On 2017/12/14 3:31, Matthew Wilcox wrote:
>> > On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
>> >> Matthew Wilcox <willy@infradead.org> writes:
>> >>
>> >>> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
>> >>>> Below information is reported by a lower kernel version, and I saw the
>> >>>> problem still exist in current version.
>> >>>
>> >>> I think you're right, but what an awful interface we have here!
>> >>> The user must not only fetch it, they must validate it separately?
>> >>> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
>> >>> Why not this:
>> >>
>> >> Why not go a step further and have get_timespec64 check for validity?
>> >> I wonder what caller doesn't want that to happen...
>> I tried this before. But I found some places call get_timespec64 in the following function.
>> If we do the check in get_timespec64, the check will be duplicated.
>> 
>> For example:
>> static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
>> ....
>> 	if (get_timespec64(&ts, tsp))
>> 		return -EFAULT;
>> 
>> 	to = &end_time;
>> 	if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
>> 
>> int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
>> {
>> 	struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};
>> 
>> 	if (!timespec64_valid(&ts))
>> 		return -EINVAL;
>
> The check is only two comparisons!  Why do we have an interface that can
> cause bugs for the sake of saving *two comparisons*?!  Can we talk about
> the cost of a cache miss versus the cost of executing these comparisons?

Any update on this?  Willy, I'd be okay with your get_valid_timespec64
patch if you wanted to formally submit that.

-Jeff

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Arnd Bergmann]

On Fri, Jan 12, 2018 at 8:49 PM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Matthew Wilcox <willy@infradead.org> writes:
>
>> On Thu, Dec 14, 2017 at 11:18:30AM +0800, Leizhen (ThunderTown) wrote:
>>> On 2017/12/14 3:31, Matthew Wilcox wrote:
>>> > On Wed, Dec 13, 2017 at 11:27:00AM -0500, Jeff Moyer wrote:
>>> >> Matthew Wilcox <willy@infradead.org> writes:
>>> >>
>>> >>> On Wed, Dec 13, 2017 at 09:42:52PM +0800, Zhen Lei wrote:
>>> >>>> Below information is reported by a lower kernel version, and I saw the
>>> >>>> problem still exist in current version.
>>> >>>
>>> >>> I think you're right, but what an awful interface we have here!
>>> >>> The user must not only fetch it, they must validate it separately?
>>> >>> And if they forget, then userspace is provoking undefined behaviour?  Ugh.
>>> >>> Why not this:
>>> >>
>>> >> Why not go a step further and have get_timespec64 check for validity?
>>> >> I wonder what caller doesn't want that to happen...
>>> I tried this before. But I found some places call get_timespec64 in the following function.
>>> If we do the check in get_timespec64, the check will be duplicated.
>>>
>>> For example:
>>> static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp,
>>> ....
>>>      if (get_timespec64(&ts, tsp))
>>>              return -EFAULT;
>>>
>>>      to = &end_time;
>>>      if (poll_select_set_timeout(to, ts.tv_sec, ts.tv_nsec))
>>>
>>> int poll_select_set_timeout(struct timespec64 *to, time64_t sec, long nsec)
>>> {
>>>      struct timespec64 ts = {.tv_sec = sec, .tv_nsec = nsec};
>>>
>>>      if (!timespec64_valid(&ts))
>>>              return -EINVAL;
>>
>> The check is only two comparisons!  Why do we have an interface that can
>> cause bugs for the sake of saving *two comparisons*?!  Can we talk about
>> the cost of a cache miss versus the cost of executing these comparisons?
>
> Any update on this?  Willy, I'd be okay with your get_valid_timespec64
> patch if you wanted to formally submit that.

I had suggested a more complete helper function at some point,
to take care of all combinations of checking/non-checking, 32/64
bit, microsecond/nanosecond, and zeroing/checking the upper 32 bits
of nanoseconds before comparing against 1 billion, but Deepa
thought that was overkill, so I didn't continue that.

For all I can tell, the get_timespec64() helper should almost always
include the check, the one exception I know is utimensat() and related
functions that may encode the special UTIME_NOW and UTIME_OMIT
constants in the nanoseconds.

        Arnd

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Matthew Wilcox]

On Mon, Mar 26, 2018 at 10:01:30PM +0200, Arnd Bergmann wrote:
> I had suggested a more complete helper function at some point,
> to take care of all combinations of checking/non-checking, 32/64
> bit, microsecond/nanosecond, and zeroing/checking the upper 32 bits
> of nanoseconds before comparing against 1 billion, but Deepa
> thought that was overkill, so I didn't continue that.

Yeah, that sounds like a nightmare to use ;-)

> For all I can tell, the get_timespec64() helper should almost always
> include the check, the one exception I know is utimensat() and related
> functions that may encode the special UTIME_NOW and UTIME_OMIT
> constants in the nanoseconds.

So do you endorse the get_valid_timespec64() patch I posted up-thread?
We can't just make get_timespec64 return an errno directly because it'll
require changing all the users.

Re: [PATCH 1/1] aio: make sure the input "timeout" value is valid

[Deepa Dinamani]

On Mon, Mar 26, 2018 at 2:55 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Mon, Mar 26, 2018 at 10:01:30PM +0200, Arnd Bergmann wrote:
>> I had suggested a more complete helper function at some point,
>> to take care of all combinations of checking/non-checking, 32/64
>> bit, microsecond/nanosecond, and zeroing/checking the upper 32 bits
>> of nanoseconds before comparing against 1 billion, but Deepa
>> thought that was overkill, so I didn't continue that.
>
> Yeah, that sounds like a nightmare to use ;-)
>
>> For all I can tell, the get_timespec64() helper should almost always
>> include the check, the one exception I know is utimensat() and related
>> functions that may encode the special UTIME_NOW and UTIME_OMIT
>> constants in the nanoseconds.
>
> So do you endorse the get_valid_timespec64() patch I posted up-thread?
> We can't just make get_timespec64 return an errno directly because it'll
> require changing all the users.

I missed this thread earlier.

I had leaned away from this idea before, because of the special cases
which don't need it. I was also trying to keep the syntax close to
copy_from_user(), which is what was there before.

We could probably just change all the
get_timespec64()/compat_get_timespec64() to do the check using a
simple coccinelle script.
I can own this.

But, this would not be needed if Arnd is posting the other helper function.
Arnd, let me know what you prefer here.

-Deepa