diff mbox series

[3/3] scsi: 3w-9xxx: rework lock timeouts

Message ID 20171110155840.1941250-3-arnd@arndb.de
State Accepted
Commit 07ffd4ce80b9a452d8c8eb154924e19499302d00
Headers show
Series [1/3] scsi: 3ware: fix 32-bit time calculations | expand

Commit Message

Arnd Bergmann Nov. 10, 2017, 3:58 p.m. UTC 
The TW_IOCTL_GET_LOCK ioctl uses do_gettimeofday() to check whether
a lock has expired. This can misbehave due to a concurrent
settimeofday() call, as it is based on 'real' time, and it
will overflow in y2038 on 32-bit architectures, producing
unexpected results when used across the overflow time.

This changes it to using monotonic time, using ktime_get()
to simplify the code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/scsi/3w-9xxx.c | 13 ++++++-------
 drivers/scsi/3w-9xxx.h |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.9.0

Comments

adam radford Nov. 10, 2017, 7:36 p.m. UTC | #1 
On Fri, Nov 10, 2017 at 7:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> The TW_IOCTL_GET_LOCK ioctl uses do_gettimeofday() to check whether

> a lock has expired. This can misbehave due to a concurrent

> settimeofday() call, as it is based on 'real' time, and it

> will overflow in y2038 on 32-bit architectures, producing

> unexpected results when used across the overflow time.

>

> This changes it to using monotonic time, using ktime_get()

> to simplify the code.

>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  drivers/scsi/3w-9xxx.c | 13 ++++++-------

>  drivers/scsi/3w-9xxx.h |  2 +-

>  2 files changed, 7 insertions(+), 8 deletions(-)

>

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c

> index b1c9bd9c1bfd..b42c9c479d4b 100644

> --- a/drivers/scsi/3w-9xxx.c

> +++ b/drivers/scsi/3w-9xxx.c

> @@ -645,8 +645,7 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long

>         TW_Command_Full *full_command_packet;

>         TW_Compatibility_Info *tw_compat_info;

>         TW_Event *event;

> -       struct timeval current_time;

> -       u32 current_time_ms;

> +       ktime_t current_time;

>         TW_Device_Extension *tw_dev = twa_device_extension_list[iminor(inode)];

>         int retval = TW_IOCTL_ERROR_OS_EFAULT;

>         void __user *argp = (void __user *)arg;

> @@ -837,17 +836,17 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long

>                 break;

>         case TW_IOCTL_GET_LOCK:

>                 tw_lock = (TW_Lock *)tw_ioctl->data_buffer;

> -               do_gettimeofday(&current_time);

> -               current_time_ms = (current_time.tv_sec * 1000) + (current_time.tv_usec / 1000);

> +               current_time = ktime_get();

>

> -               if ((tw_lock->force_flag == 1) || (tw_dev->ioctl_sem_lock == 0) || (current_time_ms >= tw_dev->ioctl_msec)) {

> +               if ((tw_lock->force_flag == 1) || (tw_dev->ioctl_sem_lock == 0) ||

> +                   ktime_after(current_time, tw_dev->ioctl_time)) {

>                         tw_dev->ioctl_sem_lock = 1;

> -                       tw_dev->ioctl_msec = current_time_ms + tw_lock->timeout_msec;

> +                       tw_dev->ioctl_time = ktime_add_ms(current_time, tw_lock->timeout_msec);

>                         tw_ioctl->driver_command.status = 0;

>                         tw_lock->time_remaining_msec = tw_lock->timeout_msec;

>                 } else {

>                         tw_ioctl->driver_command.status = TW_IOCTL_ERROR_STATUS_LOCKED;

> -                       tw_lock->time_remaining_msec = tw_dev->ioctl_msec - current_time_ms;

> +                       tw_lock->time_remaining_msec = ktime_ms_delta(tw_dev->ioctl_time, current_time);

>                 }

>                 break;

>         case TW_IOCTL_RELEASE_LOCK:

> diff --git a/drivers/scsi/3w-9xxx.h b/drivers/scsi/3w-9xxx.h

> index b6c208cc474f..d88cd3499bd5 100644

> --- a/drivers/scsi/3w-9xxx.h

> +++ b/drivers/scsi/3w-9xxx.h

> @@ -666,7 +666,7 @@ typedef struct TAG_TW_Device_Extension {

>         unsigned char           event_queue_wrapped;

>         unsigned int            error_sequence_id;

>         int                     ioctl_sem_lock;

> -       u32                     ioctl_msec;

> +       ktime_t                 ioctl_time;

>         int                     chrdev_request_id;

>         wait_queue_head_t       ioctl_wqueue;

>         struct mutex            ioctl_lock;

> --

> 2.9.0

>


Looks good...  Thanks for this fix!

Acked-by: Adam Radford <aradford@gmail.com>
Martin K. Petersen Nov. 21, 2017, 3:07 a.m. UTC | #2 
Arnd,

> The TW_IOCTL_GET_LOCK ioctl uses do_gettimeofday() to check whether a

> lock has expired. This can misbehave due to a concurrent

> settimeofday() call, as it is based on 'real' time, and it will

> overflow in y2038 on 32-bit architectures, producing unexpected

> results when used across the overflow time.

>

> This changes it to using monotonic time, using ktime_get() to simplify

> the code.


Applied to 4.16/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index b1c9bd9c1bfd..b42c9c479d4b 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -645,8 +645,7 @@  static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	TW_Command_Full *full_command_packet;
 	TW_Compatibility_Info *tw_compat_info;
 	TW_Event *event;
-	struct timeval current_time;
-	u32 current_time_ms;
+	ktime_t current_time;
 	TW_Device_Extension *tw_dev = twa_device_extension_list[iminor(inode)];
 	int retval = TW_IOCTL_ERROR_OS_EFAULT;
 	void __user *argp = (void __user *)arg;
@@ -837,17 +836,17 @@  static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 		break;
 	case TW_IOCTL_GET_LOCK:
 		tw_lock = (TW_Lock *)tw_ioctl->data_buffer;
-		do_gettimeofday(&current_time);
-		current_time_ms = (current_time.tv_sec * 1000) + (current_time.tv_usec / 1000);
+		current_time = ktime_get();
 
-		if ((tw_lock->force_flag == 1) || (tw_dev->ioctl_sem_lock == 0) || (current_time_ms >= tw_dev->ioctl_msec)) {
+		if ((tw_lock->force_flag == 1) || (tw_dev->ioctl_sem_lock == 0) ||
+		    ktime_after(current_time, tw_dev->ioctl_time)) {
 			tw_dev->ioctl_sem_lock = 1;
-			tw_dev->ioctl_msec = current_time_ms + tw_lock->timeout_msec;
+			tw_dev->ioctl_time = ktime_add_ms(current_time, tw_lock->timeout_msec);
 			tw_ioctl->driver_command.status = 0;
 			tw_lock->time_remaining_msec = tw_lock->timeout_msec;
 		} else {
 			tw_ioctl->driver_command.status = TW_IOCTL_ERROR_STATUS_LOCKED;
-			tw_lock->time_remaining_msec = tw_dev->ioctl_msec - current_time_ms;
+			tw_lock->time_remaining_msec = ktime_ms_delta(tw_dev->ioctl_time, current_time);
 		}
 		break;
 	case TW_IOCTL_RELEASE_LOCK:
diff --git a/drivers/scsi/3w-9xxx.h b/drivers/scsi/3w-9xxx.h
index b6c208cc474f..d88cd3499bd5 100644
--- a/drivers/scsi/3w-9xxx.h
+++ b/drivers/scsi/3w-9xxx.h
@@ -666,7 +666,7 @@  typedef struct TAG_TW_Device_Extension {
 	unsigned char		event_queue_wrapped;
 	unsigned int            error_sequence_id;
 	int                     ioctl_sem_lock;
-	u32                     ioctl_msec;
+	ktime_t                 ioctl_time;
 	int			chrdev_request_id;
 	wait_queue_head_t	ioctl_wqueue;
 	struct mutex		ioctl_lock;