[PATCH] ptp/ptp_clock.c: Correct input parameter range check

[PATCH] ptp/ptp_clock.c: Correct input parameter range check

[Siddaraju D H]

From: Siddaraju DH <siddarajudh@gmail.com>

The ealier implementaion used to return EINVAL for -ve adjustments
in the range -1ns to -999999999ns as these -ve numbers will fail the
unsigned comaparison against NSEC_PER_SEC. Since the tv_sec field
will be ZERO in this range, the user will not be able to specify
the signedness of adjustment through the tv_sec field. So, the user
must be allowed to specify the signedness through tv_usec/nsec
field also. All variables that holds usec/nsec in this function
are signed numbers and there is no need for that type cast.

Signed-off-by: Siddaraju DH <siddarajudh@gmail.com>
---
 drivers/ptp/ptp_clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 79bd102..ee2b35b 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -143,7 +143,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 		if (!(tx->modes & ADJ_NANO))
 			ts.tv_nsec *= 1000;
 
-		if ((unsigned long) ts.tv_nsec >= NSEC_PER_SEC)
+		if (ts.tv_nsec >= NSEC_PER_SEC)
 			return -EINVAL;
 
 		kt = timespec64_to_ktime(ts);
-- 
2.7.4

Re: [PATCH] ptp/ptp_clock.c: Correct input parameter range check

[Richard Cochran]

On Thu, Apr 04, 2019 at 05:39:58AM +0530, Siddaraju D H wrote:
> From: Siddaraju DH <siddarajudh@gmail.com>
> 
> The ealier implementaion used to return EINVAL for -ve adjustments
> in the range -1ns to -999999999ns as these -ve numbers will fail the
> unsigned comaparison against NSEC_PER_SEC. Since the tv_sec field
> will be ZERO in this range, the user will not be able to specify
> the signedness of adjustment through the tv_sec field.

NAK, the tv_sec field can be set to -1.  See the example, below.

Thanks,
Richard

void clockadj_step(clockid_t clkid, int64_t step)
{
	struct timex tx;
	int sign = 1;
	if (step < 0) {
		sign = -1;
		step *= -1;
	}
	memset(&tx, 0, sizeof(tx));
	tx.modes = ADJ_SETOFFSET | ADJ_NANO;
	tx.time.tv_sec  = sign * (step / NS_PER_SEC);
	tx.time.tv_usec = sign * (step % NS_PER_SEC);
	/*
	 * The value of a timeval is the sum of its fields, but the
	 * field tv_usec must always be non-negative.
	 */
	if (tx.time.tv_usec < 0) {
		tx.time.tv_sec  -= 1;
		tx.time.tv_usec += 1000000000;
	}
	if (clock_adjtime(clkid, &tx) < 0)
		pr_err("failed to step clock: %m");
}

Re: [PATCH] ptp/ptp_clock.c: Correct input parameter range check

[Siddaraju DH]

Sorry if there are duplicate emails in your inbox. Trying to solve "We
accept plain text only response but the message has HTML subpart"
error from mail delivery subsystem.

Thank you for the response with example Richard.
Agree, it works. So I take back the statement in my commit message
"Since the tv_sec field will be ZERO in this range, the user will not
be able to specify the signedness of adjustment through the tv_sec
field".

Specifying a -1 ns adjustment like
    tv_sec = 0
    tv_nsec = -1
looks pretty straight forward than specifying it like
    tv_sec = -1
    tv_nsec = 999999999.

And the former way of specifying the adjustment is consistent with how
we specify the values for positive adjustments.

So, the question is "why are we blocking -ve number in tv_nsec"? As
you mentioned, end of the day it's sum of tv_sec & tv_nsec.
If there is really an advantage, just to keep things clear, we could
have made tv_nsec/usec as "unsigned long" instead of "long". Right?
For me, it doesn't look good to expect/restrict a signed input to be unsigned.

Thanks,
Siddaraju DH