Skip to contentSkip to conversation

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

PATCH In review Josip Loncaric 3 participants 4 messages Aug 1, 2005 – Aug 2, 2005

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

Josip Loncaric

author
#0 lore raw

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

#1 lore raw

PATCH 1/1 net/sunrpc: fix time conversion error

Quoted 10 lines · 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.

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 <[email protected]>

---

 svcsock.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
net/sunrpc/svcsock.c +6 -2
@@ -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 */
 	}
#2 lore raw

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

Quoted 14 lines · Nishanth Aravamudan
Quoted 10 lines · 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.

Quoted 8 lines

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.

#3 lore raw

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

Quoted 19 lines · Patrick McHardy
Quoted 18 lines · Nishanth Aravamudan
Quoted 10 lines · 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.

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.

Quoted 8 lines
Quoted 4 lines

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>

Quoted 2 lines

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

Thanks,
Nish