[PATCH] Call input_set_timestamp for events injected using uinput

[PATCH] Call input_set_timestamp for events injected using uinput

[Biswarup Pal]

Currently, uinput doesn't use the input_set_timestamp API, so any
event injected using uinput is not accurately timestamped in terms of
measuring when the actual event happened. Hence, call the
input_set_timestamp API from uinput in order to provide a more
accurate sense of time for the event. Propagate only the timestamps
which are a) positive, b) within a pre-defined offset (10 secs) from
the current time, and c) not in the future.

Signed-off-by: Biswarup Pal <biswarupp@google.com>
---
 drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index f2593133e524..d98212d55108 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -33,6 +33,7 @@
 #define UINPUT_NAME		"uinput"
 #define UINPUT_BUFFER_SIZE	16
 #define UINPUT_NUM_REQUESTS	16
+#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10
 
 enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
 
@@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev,
 	return retval;
 }
 
+/*
+ * Returns true if the given timestamp is valid (i.e., if all the following
+ * conditions are satisfied), false otherwise.
+ * 1) given timestamp is positive
+ * 2) it's within the allowed offset before the current time
+ * 3) it's not in the future
+ */
+static bool is_valid_timestamp(const ktime_t timestamp)
+{
+	ktime_t zero_time;
+	ktime_t current_time;
+	ktime_t min_time;
+	ktime_t offset;
+
+	zero_time = ktime_set(0, 0);
+	if (ktime_compare(zero_time, timestamp) >= 0)
+		return false;
+
+	current_time = ktime_get();
+	offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0);
+	min_time = ktime_sub(current_time, offset);
+
+	if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time))
+		return false;
+
+	return true;
+}
+
 static ssize_t uinput_inject_events(struct uinput_device *udev,
 				    const char __user *buffer, size_t count)
 {
 	struct input_event ev;
 	size_t bytes = 0;
+	ktime_t timestamp;
 
 	if (count != 0 && count < input_event_size())
 		return -EINVAL;
@@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev,
 		if (input_event_from_user(buffer + bytes, &ev))
 			return -EFAULT;
 
+		timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC);
+		if (is_valid_timestamp(timestamp))
+			input_set_timestamp(udev->dev, timestamp);
+
 		input_event(udev->dev, ev.type, ev.code, ev.value);
 		bytes += input_event_size();
 		cond_resched();
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH] Call input_set_timestamp for events injected using uinput

[Siarhei Vishniakou]

Thanks Biswarup!

Just to add a bit of context: we were concerned with  breaking the
existing uinput usages where the caller might be setting an incorrect
value (since previously, that had no effect).
So the 10 second guard was added to fall back to the default behaviour
for those devices.

Reviewed-by: Siarhei Vishniakou <svv@google.com>

On Wed, Apr 26, 2023 at 5:03 PM Biswarup Pal <biswarupp@google.com> wrote:
>
> Currently, uinput doesn't use the input_set_timestamp API, so any
> event injected using uinput is not accurately timestamped in terms of
> measuring when the actual event happened. Hence, call the
> input_set_timestamp API from uinput in order to provide a more
> accurate sense of time for the event. Propagate only the timestamps
> which are a) positive, b) within a pre-defined offset (10 secs) from
> the current time, and c) not in the future.
>
> Signed-off-by: Biswarup Pal <biswarupp@google.com>
> ---
>  drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index f2593133e524..d98212d55108 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -33,6 +33,7 @@
>  #define UINPUT_NAME            "uinput"
>  #define UINPUT_BUFFER_SIZE     16
>  #define UINPUT_NUM_REQUESTS    16
> +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10
>
>  enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
>
> @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev,
>         return retval;
>  }
>
> +/*
> + * Returns true if the given timestamp is valid (i.e., if all the following
> + * conditions are satisfied), false otherwise.
> + * 1) given timestamp is positive
> + * 2) it's within the allowed offset before the current time
> + * 3) it's not in the future
> + */
> +static bool is_valid_timestamp(const ktime_t timestamp)
> +{
> +       ktime_t zero_time;
> +       ktime_t current_time;
> +       ktime_t min_time;
> +       ktime_t offset;
> +
> +       zero_time = ktime_set(0, 0);
> +       if (ktime_compare(zero_time, timestamp) >= 0)
> +               return false;
> +
> +       current_time = ktime_get();
> +       offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0);
> +       min_time = ktime_sub(current_time, offset);
> +
> +       if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time))
> +               return false;
> +
> +       return true;
> +}
> +
>  static ssize_t uinput_inject_events(struct uinput_device *udev,
>                                     const char __user *buffer, size_t count)
>  {
>         struct input_event ev;
>         size_t bytes = 0;
> +       ktime_t timestamp;
>
>         if (count != 0 && count < input_event_size())
>                 return -EINVAL;
> @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev,
>                 if (input_event_from_user(buffer + bytes, &ev))
>                         return -EFAULT;
>
> +               timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC);
> +               if (is_valid_timestamp(timestamp))
> +                       input_set_timestamp(udev->dev, timestamp);
> +
>                 input_event(udev->dev, ev.type, ev.code, ev.value);
>                 bytes += input_event_size();
>                 cond_resched();
> --
> 2.40.1.495.gc816e09b53d-goog
>

Re: [PATCH] Call input_set_timestamp for events injected using uinput

[Dmitry Torokhov]

Hi,

On Fri, Apr 28, 2023 at 11:38:36AM -0700, Siarhei Vishniakou wrote:
> Thanks Biswarup!

Please avoid top posting.

> 
> Just to add a bit of context: we were concerned with  breaking the
> existing uinput usages where the caller might be setting an incorrect
> value (since previously, that had no effect).
> So the 10 second guard was added to fall back to the default behaviour
> for those devices.

What is the benefit of sending this in uinput? It is not much close
to the exact time the hardware generated the event than the timestamp
generated in the input core, so I do not see why it is needed in uinput.

Thanks.

-- 
Dmitry

Re: [PATCH] Call input_set_timestamp for events injected using uinput

[Dmitry Torokhov]

On Fri, Apr 28, 2023 at 12:52:43PM -0700, Dmitry Torokhov wrote:
> Hi,
> 
> On Fri, Apr 28, 2023 at 11:38:36AM -0700, Siarhei Vishniakou wrote:
> > Thanks Biswarup!
> 
> Please avoid top posting.
> 
> > 
> > Just to add a bit of context: we were concerned with  breaking the
> > existing uinput usages where the caller might be setting an incorrect
> > value (since previously, that had no effect).
> > So the 10 second guard was added to fall back to the default behaviour
> > for those devices.
> 
> What is the benefit of sending this in uinput? It is not much close
> to the exact time the hardware generated the event than the timestamp
> generated in the input core, so I do not see why it is needed in uinput.

Ah, sorry, my apologies, I mis-parsed the code.

-- 
Dmitry

Re: [PATCH] Call input_set_timestamp for events injected using uinput

[Dmitry Torokhov]

On Thu, Apr 27, 2023 at 12:01:51AM +0000, Biswarup Pal wrote:
> Currently, uinput doesn't use the input_set_timestamp API, so any
> event injected using uinput is not accurately timestamped in terms of
> measuring when the actual event happened. Hence, call the
> input_set_timestamp API from uinput in order to provide a more
> accurate sense of time for the event. Propagate only the timestamps
> which are a) positive, b) within a pre-defined offset (10 secs) from
> the current time, and c) not in the future.

This makes sense to me. Peter, do you see any issues?

Thanks!

> 
> Signed-off-by: Biswarup Pal <biswarupp@google.com>
> ---
>  drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index f2593133e524..d98212d55108 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -33,6 +33,7 @@
>  #define UINPUT_NAME		"uinput"
>  #define UINPUT_BUFFER_SIZE	16
>  #define UINPUT_NUM_REQUESTS	16
> +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10
>  
>  enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
>  
> @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev,
>  	return retval;
>  }
>  
> +/*
> + * Returns true if the given timestamp is valid (i.e., if all the following
> + * conditions are satisfied), false otherwise.
> + * 1) given timestamp is positive
> + * 2) it's within the allowed offset before the current time
> + * 3) it's not in the future
> + */
> +static bool is_valid_timestamp(const ktime_t timestamp)
> +{
> +	ktime_t zero_time;
> +	ktime_t current_time;
> +	ktime_t min_time;
> +	ktime_t offset;
> +
> +	zero_time = ktime_set(0, 0);
> +	if (ktime_compare(zero_time, timestamp) >= 0)
> +		return false;
> +
> +	current_time = ktime_get();
> +	offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0);
> +	min_time = ktime_sub(current_time, offset);
> +
> +	if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time))
> +		return false;
> +
> +	return true;
> +}
> +
>  static ssize_t uinput_inject_events(struct uinput_device *udev,
>  				    const char __user *buffer, size_t count)
>  {
>  	struct input_event ev;
>  	size_t bytes = 0;
> +	ktime_t timestamp;
>  
>  	if (count != 0 && count < input_event_size())
>  		return -EINVAL;
> @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev,
>  		if (input_event_from_user(buffer + bytes, &ev))
>  			return -EFAULT;
>  
> +		timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC);
> +		if (is_valid_timestamp(timestamp))
> +			input_set_timestamp(udev->dev, timestamp);
> +
>  		input_event(udev->dev, ev.type, ev.code, ev.value);
>  		bytes += input_event_size();
>  		cond_resched();
> -- 
> 2.40.1.495.gc816e09b53d-goog
> 

-- 
Dmitry

Re: [PATCH] Call input_set_timestamp for events injected using uinput

[Peter Hutterer]

Thanks for the CC, I would've missed that.

On Mon, May 01, 2023 at 06:05:59PM -0700, Dmitry Torokhov wrote:
> On Thu, Apr 27, 2023 at 12:01:51AM +0000, Biswarup Pal wrote:
> > Currently, uinput doesn't use the input_set_timestamp API, so any
> > event injected using uinput is not accurately timestamped in terms of
> > measuring when the actual event happened. Hence, call the
> > input_set_timestamp API from uinput in order to provide a more
> > accurate sense of time for the event. Propagate only the timestamps
> > which are a) positive, b) within a pre-defined offset (10 secs) from
> > the current time, and c) not in the future.
> 
> This makes sense to me. Peter, do you see any issues?

nope, this looks good and has my 
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net> 

afaict any code that has compiler warnings enabled should have this on
zero anyway. It'd be really nice to pass a timestamp down to uinput but
that's obviously a lot more involved.

Cheers,
  Peter

> 
> Thanks!
> 
> > 
> > Signed-off-by: Biswarup Pal <biswarupp@google.com>
> > ---
> >  drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > index f2593133e524..d98212d55108 100644
> > --- a/drivers/input/misc/uinput.c
> > +++ b/drivers/input/misc/uinput.c
> > @@ -33,6 +33,7 @@
> >  #define UINPUT_NAME		"uinput"
> >  #define UINPUT_BUFFER_SIZE	16
> >  #define UINPUT_NUM_REQUESTS	16
> > +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10
> >  
> >  enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
> >  
> > @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev,
> >  	return retval;
> >  }
> >  
> > +/*
> > + * Returns true if the given timestamp is valid (i.e., if all the following
> > + * conditions are satisfied), false otherwise.
> > + * 1) given timestamp is positive
> > + * 2) it's within the allowed offset before the current time
> > + * 3) it's not in the future
> > + */
> > +static bool is_valid_timestamp(const ktime_t timestamp)
> > +{
> > +	ktime_t zero_time;
> > +	ktime_t current_time;
> > +	ktime_t min_time;
> > +	ktime_t offset;
> > +
> > +	zero_time = ktime_set(0, 0);
> > +	if (ktime_compare(zero_time, timestamp) >= 0)
> > +		return false;
> > +
> > +	current_time = ktime_get();
> > +	offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0);
> > +	min_time = ktime_sub(current_time, offset);
> > +
> > +	if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static ssize_t uinput_inject_events(struct uinput_device *udev,
> >  				    const char __user *buffer, size_t count)
> >  {
> >  	struct input_event ev;
> >  	size_t bytes = 0;
> > +	ktime_t timestamp;
> >  
> >  	if (count != 0 && count < input_event_size())
> >  		return -EINVAL;
> > @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev,
> >  		if (input_event_from_user(buffer + bytes, &ev))
> >  			return -EFAULT;
> >  
> > +		timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC);
> > +		if (is_valid_timestamp(timestamp))
> > +			input_set_timestamp(udev->dev, timestamp);
> > +
> >  		input_event(udev->dev, ev.type, ev.code, ev.value);
> >  		bytes += input_event_size();
> >  		cond_resched();
> > -- 
> > 2.40.1.495.gc816e09b53d-goog
> > 
> 
> -- 
> Dmitry

Re: [PATCH] Call input_set_timestamp for events injected using uinput

[Dmitry Torokhov]

On Tue, May 02, 2023 at 12:13:05PM +1000, Peter Hutterer wrote:
> Thanks for the CC, I would've missed that.
> 
> On Mon, May 01, 2023 at 06:05:59PM -0700, Dmitry Torokhov wrote:
> > On Thu, Apr 27, 2023 at 12:01:51AM +0000, Biswarup Pal wrote:
> > > Currently, uinput doesn't use the input_set_timestamp API, so any
> > > event injected using uinput is not accurately timestamped in terms of
> > > measuring when the actual event happened. Hence, call the
> > > input_set_timestamp API from uinput in order to provide a more
> > > accurate sense of time for the event. Propagate only the timestamps
> > > which are a) positive, b) within a pre-defined offset (10 secs) from
> > > the current time, and c) not in the future.
> > 
> > This makes sense to me. Peter, do you see any issues?
> 
> nope, this looks good and has my 
> Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net> 

Great, applied.

> 
> afaict any code that has compiler warnings enabled should have this on
> zero anyway. It'd be really nice to pass a timestamp down to uinput but
> that's obviously a lot more involved.
> 
> Cheers,
>   Peter
> 
> > 
> > Thanks!
> > 
> > > 
> > > Signed-off-by: Biswarup Pal <biswarupp@google.com>
> > > ---
> > >  drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > > index f2593133e524..d98212d55108 100644
> > > --- a/drivers/input/misc/uinput.c
> > > +++ b/drivers/input/misc/uinput.c
> > > @@ -33,6 +33,7 @@
> > >  #define UINPUT_NAME		"uinput"
> > >  #define UINPUT_BUFFER_SIZE	16
> > >  #define UINPUT_NUM_REQUESTS	16
> > > +#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10
> > >  
> > >  enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
> > >  
> > > @@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev,
> > >  	return retval;
> > >  }
> > >  
> > > +/*
> > > + * Returns true if the given timestamp is valid (i.e., if all the following
> > > + * conditions are satisfied), false otherwise.
> > > + * 1) given timestamp is positive
> > > + * 2) it's within the allowed offset before the current time
> > > + * 3) it's not in the future
> > > + */
> > > +static bool is_valid_timestamp(const ktime_t timestamp)
> > > +{
> > > +	ktime_t zero_time;
> > > +	ktime_t current_time;
> > > +	ktime_t min_time;
> > > +	ktime_t offset;
> > > +
> > > +	zero_time = ktime_set(0, 0);
> > > +	if (ktime_compare(zero_time, timestamp) >= 0)
> > > +		return false;
> > > +
> > > +	current_time = ktime_get();
> > > +	offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0);
> > > +	min_time = ktime_sub(current_time, offset);
> > > +
> > > +	if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time))
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  static ssize_t uinput_inject_events(struct uinput_device *udev,
> > >  				    const char __user *buffer, size_t count)
> > >  {
> > >  	struct input_event ev;
> > >  	size_t bytes = 0;
> > > +	ktime_t timestamp;
> > >  
> > >  	if (count != 0 && count < input_event_size())
> > >  		return -EINVAL;
> > > @@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev,
> > >  		if (input_event_from_user(buffer + bytes, &ev))
> > >  			return -EFAULT;
> > >  
> > > +		timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC);
> > > +		if (is_valid_timestamp(timestamp))
> > > +			input_set_timestamp(udev->dev, timestamp);
> > > +
> > >  		input_event(udev->dev, ev.type, ev.code, ev.value);
> > >  		bytes += input_event_size();
> > >  		cond_resched();
> > > -- 
> > > 2.40.1.495.gc816e09b53d-goog
> > > 
> > 
> > -- 
> > Dmitry

-- 
Dmitry