[v2] HID: wacom: Use ktime_t rather than int when dealing with timestamps

Code which interacts with timestamps needs to use the ktime_t type
returned by functions like ktime_get. The int type does not offer
enough space to store these values, and attempting to use it is a
recipe for problems. In this particular case, overflows would occur
when calculating/storing timestamps leading to incorrect values being
reported to userspace. In some cases these bad timestamps cause input
handling in userspace to appear hung.

Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
CC: stable@vger.kernel.org
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
v2: Use div_u64 to perform division to deal with ARC and ARM architectures
    (as found by the kernel test robot)

 drivers/hid/wacom_wac.c | 6 +++---
 drivers/hid/wacom_wac.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Following up since no action seems to have been taken on this patch yet.

On Thu, Jun 8, 2023 at 2:38 PM Jason Gerecke <killertofu@gmail.com> wrote:
>
> Code which interacts with timestamps needs to use the ktime_t type
> returned by functions like ktime_get. The int type does not offer
> enough space to store these values, and attempting to use it is a
> recipe for problems. In this particular case, overflows would occur
> when calculating/storing timestamps leading to incorrect values being
> reported to userspace. In some cases these bad timestamps cause input
> handling in userspace to appear hung.
>
> Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
> Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
> CC: stable@vger.kernel.org
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
> v2: Use div_u64 to perform division to deal with ARC and ARM architectures
>     (as found by the kernel test robot)
>
>  drivers/hid/wacom_wac.c | 6 +++---
>  drivers/hid/wacom_wac.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 2ccf838371343..174bf03908d7c 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1314,7 +1314,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
>         struct input_dev *pen_input = wacom->pen_input;
>         unsigned char *data = wacom->data;
>         int number_of_valid_frames = 0;
> -       int time_interval = 15000000;
> +       ktime_t time_interval = 15000000;
>         ktime_t time_packet_received = ktime_get();
>         int i;
>
> @@ -1348,7 +1348,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
>         if (number_of_valid_frames) {
>                 if (wacom->hid_data.time_delayed)
>                         time_interval = ktime_get() - wacom->hid_data.time_delayed;
> -               time_interval /= number_of_valid_frames;
> +               time_interval = div_u64(time_interval, number_of_valid_frames);
>                 wacom->hid_data.time_delayed = time_packet_received;
>         }
>
> @@ -1359,7 +1359,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
>                 bool range = frame[0] & 0x20;
>                 bool invert = frame[0] & 0x10;
>                 int frames_number_reversed = number_of_valid_frames - i - 1;
> -               int event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> +               ktime_t event_timestamp = time_packet_received - frames_number_reversed * time_interval;
>
>                 if (!valid)
>                         continue;
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 1a40bb8c5810c..ee21bb260f22f 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -324,7 +324,7 @@ struct hid_data {
>         int ps_connected;
>         bool pad_input_event_flag;
>         unsigned short sequence_number;
> -       int time_delayed;
> +       ktime_t time_delayed;
>  };
>
>  struct wacom_remote_data {
> --
> 2.41.0
>

On Wed, Jun 21, 2023 at 5:18 PM Jason Gerecke <killertofu@gmail.com> wrote:
>
> Following up since no action seems to have been taken on this patch yet.

Sorry, this went through the cracks (I seem to have a lot of cracks recently...)

>
> On Thu, Jun 8, 2023 at 2:38 PM Jason Gerecke <killertofu@gmail.com> wrote:
> >
> > Code which interacts with timestamps needs to use the ktime_t type
> > returned by functions like ktime_get. The int type does not offer
> > enough space to store these values, and attempting to use it is a
> > recipe for problems. In this particular case, overflows would occur
> > when calculating/storing timestamps leading to incorrect values being
> > reported to userspace. In some cases these bad timestamps cause input
> > handling in userspace to appear hung.

I have to ask, is this something we should consider writing a test for? :)

Also, you are missing the rev-by from Peter, not sure if the tools
will pick it up automatically then.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> >
> > Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
> > Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
> > CC: stable@vger.kernel.org
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > ---
> > v2: Use div_u64 to perform division to deal with ARC and ARM architectures
> >     (as found by the kernel test robot)
> >
> >  drivers/hid/wacom_wac.c | 6 +++---
> >  drivers/hid/wacom_wac.h | 2 +-
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > index 2ccf838371343..174bf03908d7c 100644
> > --- a/drivers/hid/wacom_wac.c
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -1314,7 +1314,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> >         struct input_dev *pen_input = wacom->pen_input;
> >         unsigned char *data = wacom->data;
> >         int number_of_valid_frames = 0;
> > -       int time_interval = 15000000;
> > +       ktime_t time_interval = 15000000;
> >         ktime_t time_packet_received = ktime_get();
> >         int i;
> >
> > @@ -1348,7 +1348,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> >         if (number_of_valid_frames) {
> >                 if (wacom->hid_data.time_delayed)
> >                         time_interval = ktime_get() - wacom->hid_data.time_delayed;
> > -               time_interval /= number_of_valid_frames;
> > +               time_interval = div_u64(time_interval, number_of_valid_frames);
> >                 wacom->hid_data.time_delayed = time_packet_received;
> >         }
> >
> > @@ -1359,7 +1359,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> >                 bool range = frame[0] & 0x20;
> >                 bool invert = frame[0] & 0x10;
> >                 int frames_number_reversed = number_of_valid_frames - i - 1;
> > -               int event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> > +               ktime_t event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> >
> >                 if (!valid)
> >                         continue;
> > diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> > index 1a40bb8c5810c..ee21bb260f22f 100644
> > --- a/drivers/hid/wacom_wac.h
> > +++ b/drivers/hid/wacom_wac.h
> > @@ -324,7 +324,7 @@ struct hid_data {
> >         int ps_connected;
> >         bool pad_input_event_flag;
> >         unsigned short sequence_number;
> > -       int time_delayed;
> > +       ktime_t time_delayed;
> >  };
> >
> >  struct wacom_remote_data {
> > --
> > 2.41.0
> >
>

On Wed, Jun 21, 2023 at 9:04 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jun 21, 2023 at 5:18 PM Jason Gerecke <killertofu@gmail.com> wrote:
> >
> > Following up since no action seems to have been taken on this patch yet.
>
> Sorry, this went through the cracks (I seem to have a lot of cracks recently...)
>
> >
> > On Thu, Jun 8, 2023 at 2:38 PM Jason Gerecke <killertofu@gmail.com> wrote:
> > >
> > > Code which interacts with timestamps needs to use the ktime_t type
> > > returned by functions like ktime_get. The int type does not offer
> > > enough space to store these values, and attempting to use it is a
> > > recipe for problems. In this particular case, overflows would occur
> > > when calculating/storing timestamps leading to incorrect values being
> > > reported to userspace. In some cases these bad timestamps cause input
> > > handling in userspace to appear hung.
>
> I have to ask, is this something we should consider writing a test for? :)
>

Very good point! I'm happy to work on such a test.

Are you open to merging this patch without delay while I write a
testcase? I don't like the idea of this (apparent) system freeze being
affecting users any longer than absolutely necessary.

> Also, you are missing the rev-by from Peter, not sure if the tools
> will pick it up automatically then.
>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>

Oof, good catch. Apologies to Peter :)

Jason

> Cheers,
> Benjamin
>
> > >
> > > Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
> > > Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
> > > CC: stable@vger.kernel.org
> > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > > ---
> > > v2: Use div_u64 to perform division to deal with ARC and ARM architectures
> > >     (as found by the kernel test robot)
> > >
> > >  drivers/hid/wacom_wac.c | 6 +++---
> > >  drivers/hid/wacom_wac.h | 2 +-
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > > index 2ccf838371343..174bf03908d7c 100644
> > > --- a/drivers/hid/wacom_wac.c
> > > +++ b/drivers/hid/wacom_wac.c
> > > @@ -1314,7 +1314,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > >         struct input_dev *pen_input = wacom->pen_input;
> > >         unsigned char *data = wacom->data;
> > >         int number_of_valid_frames = 0;
> > > -       int time_interval = 15000000;
> > > +       ktime_t time_interval = 15000000;
> > >         ktime_t time_packet_received = ktime_get();
> > >         int i;
> > >
> > > @@ -1348,7 +1348,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > >         if (number_of_valid_frames) {
> > >                 if (wacom->hid_data.time_delayed)
> > >                         time_interval = ktime_get() - wacom->hid_data.time_delayed;
> > > -               time_interval /= number_of_valid_frames;
> > > +               time_interval = div_u64(time_interval, number_of_valid_frames);
> > >                 wacom->hid_data.time_delayed = time_packet_received;
> > >         }
> > >
> > > @@ -1359,7 +1359,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > >                 bool range = frame[0] & 0x20;
> > >                 bool invert = frame[0] & 0x10;
> > >                 int frames_number_reversed = number_of_valid_frames - i - 1;
> > > -               int event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> > > +               ktime_t event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> > >
> > >                 if (!valid)
> > >                         continue;
> > > diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> > > index 1a40bb8c5810c..ee21bb260f22f 100644
> > > --- a/drivers/hid/wacom_wac.h
> > > +++ b/drivers/hid/wacom_wac.h
> > > @@ -324,7 +324,7 @@ struct hid_data {
> > >         int ps_connected;
> > >         bool pad_input_event_flag;
> > >         unsigned short sequence_number;
> > > -       int time_delayed;
> > > +       ktime_t time_delayed;
> > >  };
> > >
> > >  struct wacom_remote_data {
> > > --
> > > 2.41.0
> > >
> >
>

From: Benjamin Tissoires <bentiss@kernel.org>

On Thu, 08 Jun 2023 14:38:28 -0700, Jason Gerecke wrote:
> Code which interacts with timestamps needs to use the ktime_t type
> returned by functions like ktime_get. The int type does not offer
> enough space to store these values, and attempting to use it is a
> recipe for problems. In this particular case, overflows would occur
> when calculating/storing timestamps leading to incorrect values being
> reported to userspace. In some cases these bad timestamps cause input
> handling in userspace to appear hung.
> 
> [...]

Updated the "from" email from your patch.
It would be nice if you could fix your workflow (I know you well enough
to know what is your gmail address, but not having to fix this by hand
would be preferable)

Applied to hid/hid.git (for-6.5/wacom), thanks!

[1/1] HID: wacom: Use ktime_t rather than int when dealing with timestamps
      https://git.kernel.org/hid/hid/c/9a6c0e28e215

Cheers,