lkml.org 
[lkml]   [2021]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@opensource.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

    \
     
     \ /
      Last update: 2021-09-16 22:58    [W:5.086 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site