[RFC] time: Improve negative offset handling in timekeeping_inject_offset

The adjtimex() ADJ_SETOFFSET feature allows a offset in timespec
format to be added to the current time. This value can be positive
or negative. However, representing a negative value in a timespec
can be confusing, as there may be numerous ways to represent the
same amount of time.

Positive values are most obviously represented:
1)	{ 0,  500000000}
2)	{ 3,  0}
3)	{ 3,  500000000}

While negative values are more complex and confusing:
4)	{-5,  0}
5)	{ 0, -500000000}
6)	{-5, -500000000}
7)	{-6,  500000000}

Note that the last two values (#6 and #7) actually represent the
same amount of time.

In timekeeping_inject_offset, a likely naieve validation check
(implemented by me) on the timespec value resulted in -EINVAL
being returned if the nsec portion of the timespec was negative.

This resulted in the ADJ_SETOFFSET interface to consider examples
representations of {sec - 1, NSEC_PER_SEC + nsec} for negative
values (like example #7 above).

Initially I suspected the extra logic for underflow handling
was the reason this was done, but in looking at it,
set_normalized_timespec() handles both overflows and underflows
properly.

Thus this patch would allows for all of the representations
above to be considered valid.

There is of course, one missing example from the list above:
8)	{ 4, -500000000}

One could reasonably argue that examples #8 and #7 are simply
invalid timespecs, and we should have a rule:
* If tv_sec is nonzero, then the signs must agree.

I fully agree with this, but since the existing interface
only accepts #7 style negative timespecs, we have to continue
to support that style for this interface.

Another possible view is that the rule that the tv_nsec
value always be [0,1e9). And that while maybe non-intuitive,
the #7 style representations are valid and the existing
interface is correct, thus no further change is needed.

Thoughts? Comments?

I still need to do some further validation on this patch to
make sure it doesn't have any corner cases that breaks normal
time handling. But I wanted to get it out for wider discussion.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Reported-by: Kay Sievers <kay@vrfy.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On Fri, Feb 21, 2014 at 2:54 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 21 Feb 2014, John Stultz wrote:
>> I fully agree with this, but since the existing interface
>> only accepts #7 style negative timespecs, we have to continue
>> to support that style for this interface.
>>
>> Another possible view is that the rule that the tv_nsec
>> value always be [0,1e9). And that while maybe non-intuitive,
>> the #7 style representations are valid and the existing
>> interface is correct, thus no further change is needed.
>
> We have the requirement all over the place in the kernel to use
> normalized timespecs in the #7 form where: 0 <= tv_nsec < 1e9
>
[snip]
> If you have the 0 <= tv_nsec < 1e9 enforcement, then you catch that
> issue way before the add actually makes the tv_nsec value negative and
> causes some undechifferable wreckage.
>
> Sure you might argue that the requirement is:
>
>      -1e9 < tv_nsec < 1e9
>
> but then you need to allow all combinations of signs of tv_sec/tv_nsec
> just for compatibility reasons.
>
> Sure our normalize function can cope with that, but where is the
> point?
>
> We already enforce the 0 <= tv_nsec < 1e9 on all timespec interfaces
> (kernel interal and syscalls), which in turn forces people to use
> timespec_add/sub_ns or timespec_normalize.

Though we also require timespecs to be for positive intervals (at
least from a user-space side), so this case doesn't have a whole bunch
of precedent to follow.

And as an in-kernel counter example, I think the wall_to_monotonic
timespec is represented in {-1, -500} style.


> Why can't the adjtimex folks not handle that? They already have to
> handle the kernel readouts which are in the normalized form. So what's
> the problem to feed their computational value through
> normalize/sub/add whatever before handing it to the kernel.

Having libc handle the translation is indeed another option (one
Richard already brought up in private).  It rubs me a little bit the
wrong way as its fairly easy to handle this in kernel and then we
don't have compatability issues depending on what the libc
implementation does.

[snip]
> I really prefer that people use proper helper functions to
> add/sub/normalize timespecs into a single representation instead of
> having to look at a gazillion of permutations of the same unparseable
> crap.
>
> Aside of that, if we allow that for adjtimex, then how do we argue the
> restriction on all other timespec related interfaces?

I'd say the rule that the signs must agree is a good one to start out
with. However, in the case with this interface, we allow for the
awkward {-3,500} style values to be compatible with earlier releases.

But yes, we can just leave it as is, and it is a bit academic. But
mostly for me this is just about making the interface a bit more
intuitive when working with negative relative intervals, and maybe
more importantly making clear the precedent should any other userspace
interface do something similar.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 02/22/2014 02:01 AM, Thomas Gleixner wrote:
> On Fri, 21 Feb 2014, John Stultz wrote:
>> But yes, we can just leave it as is, and it is a bit academic. But
> To be honest, I was mostly arguing due to the academic nature. :)
>
> The important point is that we restrict it to -1e9 < tv_nsec <
> 1e9. The signed/unsigned combos are not that interesting as long as
> our internal helpers cope nicely with that.

And for this patch it does check abs(tv_nsec) is sane.

I still think the patch is a good idea, so I'll submit it again after
further testing. But if folks object, then I'll drop it.

thanks
-john

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/