| Message ID | 20151030083040.GA31741@tina-laptop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, 2015-10-30 at 01:30 -0700, Tina Ruchandani wrote: > Function stex_gettime uses 'struct timeval' whose tv_sec value > will overflow on 32-bit systems in year 2038 and beyond. This patch > replaces the use of struct timeval and do_gettimeofday with > ktime_get_real_seconds, which returns a 64-bit seconds value. Thanks for the conversion. Can you please check if other (scsi) drivers have the same y2038 issues? A quick "git grep do_gettimeofday drivers/scsi/ | wc -l" reveals 30 occurrences (of cause not all are problematic). Other than that Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Thanks for the conversion. Can you please check if other (scsi) drivers > have the same y2038 issues? A quick "git grep do_gettimeofday > drivers/scsi/ | wc -l" reveals 30 occurrences (of cause not all are > problematic). > Hi Johannes, Yes, there are quite a few occurrences of timeval within scsi. I had sent some of the trivial back in the Feb-May 2015 period, and they were ack-ed by my then mentor and a couple of other people, but not merged or ack-ed by someone from linux-scsi. Until today, I thought using "RESEND" would be impolite, but now I will resend the other ones as well. Arnd Bergmann is leading this effort and may have more insightful comments. Thanks, Tina -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 09:30 AM, Tina Ruchandani wrote: > Function stex_gettime uses 'struct timeval' whose tv_sec value > will overflow on 32-bit systems in year 2038 and beyond. This patch > replaces the use of struct timeval and do_gettimeofday with > ktime_get_real_seconds, which returns a 64-bit seconds value. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com> > -- > Changes in v3: > - Remove stex_gettime altogether, directly assign the timestamp. > Changes in v2: > - Change subject line to indicate that the patch is restricted to stex driver. > --- > --- > drivers/scsi/stex.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c > index 98a62bc..84e196e 100644 > --- a/drivers/scsi/stex.c > +++ b/drivers/scsi/stex.c > @@ -25,6 +25,7 @@ > #include <linux/types.h> > #include <linux/module.h> > #include <linux/spinlock.h> > +#include <linux/ktime.h> > #include <asm/io.h> > #include <asm/irq.h> > #include <asm/byteorder.h> > @@ -362,14 +363,6 @@ MODULE_DESCRIPTION("Promise Technology SuperTrak EX Controllers"); > MODULE_LICENSE("GPL"); > MODULE_VERSION(ST_DRIVER_VERSION); > > -static void stex_gettime(__le64 *time) > -{ > - struct timeval tv; > - > - do_gettimeofday(&tv); > - *time = cpu_to_le64(tv.tv_sec); > -} > - > static struct status_msg *stex_get_status(struct st_hba *hba) > { > struct status_msg *status = hba->status_buffer + hba->status_tail; > @@ -1002,7 +995,7 @@ static int stex_common_handshake(struct st_hba *hba) > h->req_cnt = cpu_to_le16(hba->rq_count+1); > h->status_sz = cpu_to_le16(sizeof(struct status_msg)); > h->status_cnt = cpu_to_le16(hba->sts_count+1); > - stex_gettime(&h->hosttime); > + h->hosttime = cpu_to_le64(ktime_get_real_seconds()); > h->partner_type = HMU_PARTNER_TYPE; > if (hba->extra_offset) { > h->extra_offset = cpu_to_le32(hba->extra_offset); > @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba) > h->req_cnt = cpu_to_le16(hba->rq_count+1); > h->status_sz = cpu_to_le16(sizeof(struct status_msg)); > h->status_cnt = cpu_to_le16(hba->sts_count+1); > - stex_gettime(&h->hosttime); > + h->hosttime = cpu_to_le64(ktime_get_real_seconds()); > h->partner_type = HMU_PARTNER_TYPE; > h->extra_offset = h->extra_size = 0; > scratch_size = (hba->sts_count+1)*sizeof(u32); > Just remove 'hosttime' altogether. It serves no purpose whatsoever. Cheers, Hannes
On Friday 30 October 2015 12:58:33 Hannes Reinecke wrote: > > @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba) > > h->req_cnt = cpu_to_le16(hba->rq_count+1); > > h->status_sz = cpu_to_le16(sizeof(struct status_msg)); > > h->status_cnt = cpu_to_le16(hba->sts_count+1); > > - stex_gettime(&h->hosttime); > > + h->hosttime = cpu_to_le64(ktime_get_real_seconds()); > > h->partner_type = HMU_PARTNER_TYPE; > > h->extra_offset = h->extra_size = 0; > > scratch_size = (hba->sts_count+1)*sizeof(u32); > > > Just remove 'hosttime' altogether. It serves no purpose whatsoever. > > Are you sure? It is defined in a struct that is shared with the HBA as part of hba->dma_mem, so unless you have access to the firmware, it's hard to guess what it might be used for inside of the firmware. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 30 October 2015 01:54:10 Tina Ruchandani wrote: > > > > Thanks for the conversion. Can you please check if other (scsi) drivers > > have the same y2038 issues? A quick "git grep do_gettimeofday > > drivers/scsi/ | wc -l" reveals 30 occurrences (of cause not all are > > problematic). In fact all of them are problematic, just for different reasons. * Some drivers actually overflow in 2038 in a way that causes problems in those drivers. These obviously need to be fixed right away. * A second class of drivers pass time_t/timeval/timespec values to or from user space. Even in cases where the absolute numbers are small (monotonic times, or time intervals), we have to change them to be able to deal with 32-bit user space that will be compiled against a modified libc using 64-bit time_t. * All other driver are likely not broken, but we want to change them anyway, to annotate the fact that we have looked at them. My goal is to remove the definition of time_t (and all derived structures) from the kernel once all drivers have been converted, to ensure that we are not adding new broken users, and to have a reasonable confidence that we have in actually found the ones that were wrong. > Hi Johannes, > Yes, there are quite a few occurrences of timeval within scsi. I had > sent some of the trivial back in the Feb-May 2015 period, and they > were ack-ed by my then mentor and a couple of other people, but not > merged or ack-ed by someone from linux-scsi. Until today, I thought > using "RESEND" would be impolite, but now I will resend the other ones > as well. Arnd Bergmann is leading this effort and may have more > insightful comments. > I provided a "Reviewed-by" tag in https://lkml.org/lkml/2015/5/5/201 . Normally, when patches get picked up directly from the list, the person who merges it should add the tags directly. However, if you have to re-send the patch (with or without small modifications), you should add that tag after your Signed-off-by, so it does not get lost when the new patch is applied. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 01:45 PM, Arnd Bergmann wrote: > On Friday 30 October 2015 12:58:33 Hannes Reinecke wrote: >>> @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba) >>> h->req_cnt = cpu_to_le16(hba->rq_count+1); >>> h->status_sz = cpu_to_le16(sizeof(struct status_msg)); >>> h->status_cnt = cpu_to_le16(hba->sts_count+1); >>> - stex_gettime(&h->hosttime); >>> + h->hosttime = cpu_to_le64(ktime_get_real_seconds()); >>> h->partner_type = HMU_PARTNER_TYPE; >>> h->extra_offset = h->extra_size = 0; >>> scratch_size = (hba->sts_count+1)*sizeof(u32); >>> >> Just remove 'hosttime' altogether. It serves no purpose whatsoever. >> >> > > Are you sure? It is defined in a struct that is shared with the HBA > as part of hba->dma_mem, so unless you have access to the firmware, > it's hard to guess what it might be used for inside of the firmware. > Ah, you're right. Okay, so we'll need to convert it. Cheers, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 30 October 2015 01:30:40 Tina Ruchandani wrote: > Function stex_gettime uses 'struct timeval' whose tv_sec value > will overflow on 32-bit systems in year 2038 and beyond. This patch > replaces the use of struct timeval and do_gettimeofday with > ktime_get_real_seconds, which returns a 64-bit seconds value. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com> > For reference Reviewed-by: Arnd Bergmann <arnd@arndb.de> I'm putting this into my y2038 tree so I don't forget it, but I'd hope that James can apply it into scsi and I'll drop it again. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c index 98a62bc..84e196e 100644 --- a/drivers/scsi/stex.c +++ b/drivers/scsi/stex.c @@ -25,6 +25,7 @@ #include <linux/types.h> #include <linux/module.h> #include <linux/spinlock.h> +#include <linux/ktime.h> #include <asm/io.h> #include <asm/irq.h> #include <asm/byteorder.h> @@ -362,14 +363,6 @@ MODULE_DESCRIPTION("Promise Technology SuperTrak EX Controllers"); MODULE_LICENSE("GPL"); MODULE_VERSION(ST_DRIVER_VERSION); -static void stex_gettime(__le64 *time) -{ - struct timeval tv; - - do_gettimeofday(&tv); - *time = cpu_to_le64(tv.tv_sec); -} - static struct status_msg *stex_get_status(struct st_hba *hba) { struct status_msg *status = hba->status_buffer + hba->status_tail; @@ -1002,7 +995,7 @@ static int stex_common_handshake(struct st_hba *hba) h->req_cnt = cpu_to_le16(hba->rq_count+1); h->status_sz = cpu_to_le16(sizeof(struct status_msg)); h->status_cnt = cpu_to_le16(hba->sts_count+1); - stex_gettime(&h->hosttime); + h->hosttime = cpu_to_le64(ktime_get_real_seconds()); h->partner_type = HMU_PARTNER_TYPE; if (hba->extra_offset) { h->extra_offset = cpu_to_le32(hba->extra_offset); @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba) h->req_cnt = cpu_to_le16(hba->rq_count+1); h->status_sz = cpu_to_le16(sizeof(struct status_msg)); h->status_cnt = cpu_to_le16(hba->sts_count+1); - stex_gettime(&h->hosttime); + h->hosttime = cpu_to_le64(ktime_get_real_seconds()); h->partner_type = HMU_PARTNER_TYPE; h->extra_offset = h->extra_size = 0; scratch_size = (hba->sts_count+1)*sizeof(u32);
Function stex_gettime uses 'struct timeval' whose tv_sec value will overflow on 32-bit systems in year 2038 and beyond. This patch replaces the use of struct timeval and do_gettimeofday with ktime_get_real_seconds, which returns a 64-bit seconds value. Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com> -- Changes in v3: - Remove stex_gettime altogether, directly assign the timestamp. Changes in v2: - Change subject line to indicate that the patch is restricted to stex driver. --- --- drivers/scsi/stex.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)