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