Time conversion error in linux-2.6.11.10/net/sunrpoc/svcsock.c

Time conversion error in linux-2.6.11.10/net/sunrpoc/svcsock.c

[Josip Loncaric]

Line 589 of linux-2.6.11.10/net/sunrpc/svcsock.c is obviously wrong:

                 skb->stamp.tv_usec = xtime.tv_nsec * 1000;

To convert nsec to usec, one should divide instead of multiplying:

                 skb->stamp.tv_usec = xtime.tv_nsec / 1000;

The same bug could be present in the latest kernels, although I haven't 
checked.  This bug makes svc_udp_recvfrom() timestamps incorrect.

Sincerely,
Josip

[PATCH] net/sunrpc: fix time conversion error

[Nishanth Aravamudan]

On 01.08.2005 [15:11:48 -0600], Josip Loncaric wrote:
> Line 589 of linux-2.6.11.10/net/sunrpc/svcsock.c is obviously wrong:
> 
>                 skb->stamp.tv_usec = xtime.tv_nsec * 1000;
> 
> To convert nsec to usec, one should divide instead of multiplying:
> 
>                 skb->stamp.tv_usec = xtime.tv_nsec / 1000;
> 
> The same bug could be present in the latest kernels, although I haven't 
> checked.  This bug makes svc_udp_recvfrom() timestamps incorrect.

Agreed, the conversion is wrong. I think the code is buggy period, as it
accesses xtime without grabbing the xtime_lock first. Following patch
should fix both issues.

Description: This function incorrectly multiplies a nanosecond value by
1000, instead of dividing by 1000, to obtain a corresponding microsecond
value. Fix the math. Also, the function incorrectly accesses xtime
without using the xtime_lock. Fixed as well. Patch is compile-tested.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

---

 svcsock.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

--- 2.6.13-rc4/net/sunrpc/svcsock.c	2005-07-29 14:11:50.000000000 -0700
+++ 2.6.13-rc4-dev/net/sunrpc/svcsock.c	2005-08-01 15:51:11.000000000 -0700
@@ -559,6 +559,7 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 	struct svc_serv	*serv = svsk->sk_server;
 	struct sk_buff	*skb;
 	int		err, len;
+        unsigned long	seq;
 
 	if (test_and_clear_bit(SK_CHNGBUF, &svsk->sk_flags))
 	    /* udp sockets need large rcvbuf as all pending
@@ -585,8 +586,11 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
 		dprintk("svc: recvfrom returned error %d\n", -err);
 	}
 	if (skb->stamp.tv_sec == 0) {
-		skb->stamp.tv_sec = xtime.tv_sec; 
-		skb->stamp.tv_usec = xtime.tv_nsec * 1000; 
+		do {
+			seq = read_seqbegin(&xtime_lock);
+			skb->stamp.tv_sec = xtime.tv_sec; 
+			skb->stamp.tv_usec = xtime.tv_nsec / 1000; 
+		} while (read_seqretry(&xtime_lock, seq));
 		/* Don't enable netstamp, sunrpc doesn't 
 		   need that much accuracy */
 	}

Re: [PATCH] net/sunrpc: fix time conversion error

[Patrick McHardy]

Nishanth Aravamudan wrote:
> On 01.08.2005 [15:11:48 -0600], Josip Loncaric wrote:
> 
>>Line 589 of linux-2.6.11.10/net/sunrpc/svcsock.c is obviously wrong:
>>
>>                skb->stamp.tv_usec = xtime.tv_nsec * 1000;
>>
>>To convert nsec to usec, one should divide instead of multiplying:
>>
>>                skb->stamp.tv_usec = xtime.tv_nsec / 1000;
>>
>>The same bug could be present in the latest kernels, although I haven't 
>>checked.  This bug makes svc_udp_recvfrom() timestamps incorrect.
> 
> 
> Agreed, the conversion is wrong. I think the code is buggy period, as it
> accesses xtime without grabbing the xtime_lock first. Following patch
> should fix both issues.
> 
> Description: This function incorrectly multiplies a nanosecond value by
> 1000, instead of dividing by 1000, to obtain a corresponding microsecond
> value. Fix the math. Also, the function incorrectly accesses xtime
> without using the xtime_lock. Fixed as well. Patch is compile-tested.

Depending on in which release you want this patch included, you might
want to redo it against Dave's net-2.6.14 tree. It includes a patch that
changes skb->stamp to an offset against a base timestamp.

Regards
Patrick

PS: I'll submit the patch to break compilation for unconverted users
ASAP.

Re: [PATCH] net/sunrpc: fix time conversion error

[Nishanth Aravamudan]

On 02.08.2005 [02:03:59 +0200], Patrick McHardy wrote:
> Nishanth Aravamudan wrote:
> > On 01.08.2005 [15:11:48 -0600], Josip Loncaric wrote:
> > 
> >>Line 589 of linux-2.6.11.10/net/sunrpc/svcsock.c is obviously wrong:
> >>
> >>                skb->stamp.tv_usec = xtime.tv_nsec * 1000;
> >>
> >>To convert nsec to usec, one should divide instead of multiplying:
> >>
> >>                skb->stamp.tv_usec = xtime.tv_nsec / 1000;
> >>
> >>The same bug could be present in the latest kernels, although I haven't 
> >>checked.  This bug makes svc_udp_recvfrom() timestamps incorrect.
> > 
> > 
> > Agreed, the conversion is wrong. I think the code is buggy period, as it
> > accesses xtime without grabbing the xtime_lock first. Following patch
> > should fix both issues.
> > 
> > Description: This function incorrectly multiplies a nanosecond value by
> > 1000, instead of dividing by 1000, to obtain a corresponding microsecond
> > value. Fix the math. Also, the function incorrectly accesses xtime
> > without using the xtime_lock. Fixed as well. Patch is compile-tested.
> 
> Depending on in which release you want this patch included, you might
> want to redo it against Dave's net-2.6.14 tree. It includes a patch that
> changes skb->stamp to an offset against a base timestamp.

Ok, I'll try and get around to rediff'ing tonight or tomorrow. Thanks
for the feedback!

<snip>

> PS: I'll submit the patch to break compilation for unconverted users
> ASAP.

Thanks,
Nish