linux-kernel - Re: [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

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]

Date:   Mon, 20 Mar 2017 13:39:21 -0700
From:   Deepa Dinamani <deepa.kernel@...il.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        John Stultz <john.stultz@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        y2038 Mailman List <y2038@...ts.linaro.org>
Subject: Re: [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to
 use timespec64

> Please do not explain WHAT the patch is doing. We can see that from the
> diff itself. What's important is the WHY. A good changelog is structured in
> paragraphs, which explain the context, the problem and the solution. Please
> read Documentation/process/submitting-patches.rst. Let me give you an
> example.
>
>   struct timespec is not Y2038 safe on 32 bit machines and needs to be
>   replaced with struct timespec64.
>
>   The posix clock functions use struct timespec directly and through struct
>   itimerspec.
>
>   Change all function prototypes to use timespec64 and itimerspec64 and fix
>   up all implementations.
>
> That gives all the information a reviewer or someone who is looking at the
> commit later needs: context, problem scope and solution.
>
> Hmm?

Thanks for the guidance. Will fix the changelog along these lines.

>>  /* posix clock implementation */
>>
>> -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
>> +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp)
>
> That's a pretty pointless exercise. getres() returns the resolution of the
> clock which obviously can never be affected by Y2038.

True, tv_sec does not need to be more than 32 bits here.
We plan to limit the use of struct timespec to existing user interfaces only.
This is the reason for the change.

>>  static int pc_clock_settime(clockid_t id, const struct timespec *ts)
>>  {
>>       struct posix_clock_desc cd;
>> +     struct timespec64 ts64 = timespec_to_timespec64(*ts);
>>       int err;
>
> Please order the variables as a reverse fir tree sorted by length.

Will take care of these orderings.

>         struct timespec64 ts64 = timespec_to_timespec64(*ts);
>         struct posix_clock_desc cd;
>         int err;
>
> That's way simpler to parse than the above random length odering.
>
>> @@ -418,14 +427,19 @@ static int pc_timer_settime(struct k_itimer *kit, int flags,
>>  {
>>       clockid_t id = kit->it_clock;
>>       struct posix_clock_desc cd;
>> +     struct itimerspec64 old64;
>> +     struct itimerspec64 ts64 = itimerspec_to_itimerspec64(ts);

Thanks,
Deepa