linux-kernel - Re: [PATCH v2 2/2] MIPS: Convert update_persistent_clock() to update_persistent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a2eXo6MkLLkzQVDtk_W+2x+68iphsX3MXAtzpTYDaREiQ@mail.gmail.com>
Date:   Sat, 5 May 2018 23:04:18 -0400
From:   Arnd Bergmann <arnd@...db.de>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     "Maciej W. Rozycki" <macro@...ux-mips.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        James Hogan <jhogan@...nel.org>, chenhc@...ote.com,
        Kate Stewart <kstewart@...uxfoundation.org>,
        gregkh <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Philippe Ombredanne <pombredanne@...b.com>,
        Mark Brown <broonie@...nel.org>,
        Paul Burton <paul.burton@...s.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        "open list:RALINK MIPS ARCHITECTURE" <linux-mips@...ux-mips.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] MIPS: Convert update_persistent_clock() to update_persistent_clock64()

On Fri, May 4, 2018 at 3:07 AM, Baolin Wang <baolin.wang@...aro.org> wrote:
> Since struct timespec is not y2038 safe on 32bit machines, this patch
> converts update_persistent_clock() to update_persistent_clock64() using
> struct timespec64.
>
> The rtc_mips_set_time() and rtc_mips_set_mmss() interfaces were using
> 'unsigned long' type that is not y2038 safe on 32bit machines, moreover
> there is only one platform implementing rtc_mips_set_time() and two
> platforms implementing rtc_mips_set_mmss(), so we can just make them each
> implement update_persistent_clock64() directly, to get that helper out
> of the common mips code by removing rtc_mips_set_time() and
> rtc_mips_set_mmss() interfaces.
>
> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>

Looks good overall, but I still found a bug and one minor issue. With
those fixed,

Acked-by: Arnd Bergmann <arnd@...db.de>

> diff --git a/arch/mips/dec/time.c b/arch/mips/dec/time.c
> index 9e992cf..934db6f 100644
> --- a/arch/mips/dec/time.c
> +++ b/arch/mips/dec/time.c
> @@ -59,14 +59,15 @@ void read_persistent_clock64(struct timespec64 *ts)
>  }
>
>  /*
> - * In order to set the CMOS clock precisely, rtc_mips_set_mmss has to
> + * In order to set the CMOS clock precisely, update_persistent_clock64 has to
>   * be called 500 ms after the second nowtime has started, because when
>   * nowtime is written into the registers of the CMOS clock, it will
>   * jump to the next second precisely 500 ms later.  Check the Dallas
>   * DS1287 data sheet for details.
>   */
> -int rtc_mips_set_mmss(unsigned long nowtime)
> +int update_persistent_clock64(struct timespec64 now)
>  {
> +       time64_t nowtime = now.tv_sec;
>         int retval = 0;
>         int real_seconds, real_minutes, cmos_minutes;
>         unsigned char save_control, save_freq_select;


It looks like you now get an invalid 64-bit division in here,
you have to change it to either use div_u64_rem() or possibly
time64_to_tm() or rtc_time64_to_tm() (the latter requires
CONFIG_RTC_LIB).

> diff --git a/arch/mips/lasat/ds1603.c b/arch/mips/lasat/ds1603.c
> index d75c887..061815e 100644
> --- a/arch/mips/lasat/ds1603.c
> +++ b/arch/mips/lasat/ds1603.c
> @@ -98,7 +98,7 @@ static void rtc_write_byte(unsigned int byte)
>         }
>  }
>
> -static void rtc_write_word(unsigned long word)
> +static void rtc_write_word(time64_t word)
>  {
>         int i;
>

I would say this function should take a 'u32' argument (or keep the
unsigned long) to match the name and implementation, but then have a
type cast in the caller with a comment about the loss of range and overflow
in y2106.

> diff --git a/arch/mips/lasat/sysctl.c b/arch/mips/lasat/sysctl.c
> index 6f74224..76f7b62 100644
> --- a/arch/mips/lasat/sysctl.c
> +++ b/arch/mips/lasat/sysctl.c
> @@ -73,8 +73,12 @@ int proc_dolasatrtc(struct ctl_table *table, int write,
>         if (r)
>                 return r;
>
> -       if (write)
> -               rtc_mips_set_mmss(rtctmp);
> +       if (write) {
> +               ts.tv_sec = rtctmp;
> +               ts.tv_nsec = 0;
> +
> +               update_persistent_clock64(ts);
> +       }
>
... and probably also a comment here to explain that we can't actually use
the full 64-bit range because of HW limits.

         Arnd