Date:   Thu, 16 Sep 2021 22:57:56 +0200
From:   Arnd Bergmann <arnd@...nel.org>
To:     OPENSOURCE Lukas Hannen 
        <lukas.hannen@...nsource.tttech-industrial.com>
Cc:     "EMC: linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "# 3.4.x" <stable@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 5.14 298/334] time: Handle negative seconds correctly in timespec64_to_ns()

On Thu, Sep 16, 2021 at 6:50 PM OPENSOURCE Lukas Hannen
<lukas.hannen@...nsource.tttech-industrial.com> wrote:
>
> > I can see how this helps the ptp_clock_adjtime() users, but I just double-checked what
> > other callers exist, and I think it introduces a regression in setitimer(), which does
> >
> >         nval = timespec64_to_ns(&value->it_value);
> >         ninterval = timespec64_to_ns(&value->it_interval);
> >
> > without any further range checking that I could find. Setting timers with negative intervals
> > sounds like a bad idea, and interpreting negative it_value as a past time instead of KTIME_SEC_MAX
> > sounds like an unintended interface change.
>
> Hello Arnd,
>
> I have looked into this, and it seems like before your
> commit bd40a175769d ("y2038: itimer: change implementation to timespec64")
> the "clamping and converting to positive ns" was done using timeval_to_ktime()
> and ktime_to_ns().

Actually, looking back at this change, I see that there was an
explicit timeval_valid()
check in get_itimerval(), and this was moved around but is still
there, I guess we're
good for this syscall, and the user-visible behavior never actually changed.

> When Commit c5021b2547ad ( "time: Prevent undefined behaviour in timespec64_to_ns()" )
> put this functionally into timespec64_to_ns(), the patchnotes mentioned the clamping to
> KTIME_SEC_MAX, but did not mention the explicit need to return KTIME_SEC_MAX for any
> negative input.

Right.

> Since timespec64_to_ns() is widely used in drivers, where negative nanosecond values are
> quite sensible, I propose to view both of the effects I mentioned above as separate functionalities,
>
> either to be implemented as separate functions in time64.h (named, for example, timespec64_to_ns()
> and timespec64_to_positive_ns),

I don't mind having the common version work the way it does after your patch, I
was only worried about silently changing the behavior for a documented syscall.

> or alternatively, since the setitimer() code seems to be the only one not expecting negative nanoseconds
> out of timespec64_to_ns() when fed negative input, the clamping of negative nanosecond values
> to KTIME_SEC_MAX to be moved into the setitimer() code, and timespec64_to_ns() to be changed
> according to the patch I submitted.
>
> Both of those alternatives seem trivial and I can send in patches for both of them,
> but since this is more a matter of style I would like to hear your opinions on this beforehand.

It looks like we don't have to do anything for setitimer(), but that
was just the first one that
I happened to look at. Did you check the other instances to see if
anything might be going
wrong there? If they are all good,  then I have no other concerns and
we should probably
put your fix back into the stable kernels (Greg has just reverted it
after my initial mail).

I went through all instances other than the ptp related ones, and I'm
pretty confident
that they are all good now, in each case either your patch fixes a bug
or the value is
already known to be positive and it doesn't matter. Are you confident
that the ptp
instances are all good as well?

I did stumble over one small detail:

        if (ts->tv_sec <= KTIME_SEC_MIN)
                return KTIME_MIN;

I think this is not entirely correct for the case of tv_sec==KTIME_SEC_MIN
with a nonzero tv_nsec, as we now round down to the full second. Not sure
if that's worth changing, as we also round up for any value between
KTIME_SEC_MAX*NSEC_PER_SEC and KTIME_MAX, or between
KTIME_MIN and KTIME_SEC_MIN*NSEC_PER_SEC.
In practice I guess we care very little about the last nanosecond in the corner
cases.

        Arnd

Powered by blists - more mailing lists