Skip to contentSkip to conversation

UDP: Always checksum even if without socket filter

PATCH In review Wang Chen 9 participants 36 messages Nov 16, 2007 – Nov 29, 2007

Subsystems (4)
Networking Network drivers Documentation UDP
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

PATCH 1/2 UDP: Always checksum even if without socket filter

Dave,

The current kernel doesn't verify the udp checksum if user doesn't set a socket filter.It's fine for LAN. But for WAN, it's not a good option. Shall we fix it? Below is the patch to make udp checksum be always available.

[IPV4] UDP: Always checksum even if without socket filter Make udp checksum be always available even if without socket filter.

Signed-off-by Wang Chen <[email protected]>
---
 udp.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
net/ipv4/udp.c +0 -0
@@ -1011,10 +1011,8 @@ int udp_queue_rcv_skb(struct sock * sk, 
 		}

	}

- if (sk->sk_filter) {
- if (udp_lib_checksum_complete(skb))
- goto drop;
- }
+ if (udp_lib_checksum_complete(skb))
+ goto drop;

	if ((rc = sock_queue_rcv_skb(sk,skb)) < 0) {
		/* Note that an ENOMEM error is charged twice */
maintainer
#1 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 16 Nov 2007 10:46:26 +0800

Quoted 9 lines

The current kernel doesn't verify the udp checksum if user doesn't set a socket filter.It's fine for LAN. But for WAN, it's not a good option. Shall we fix it? Below is the patch to make udp checksum be always available.

[IPV4] UDP: Always checksum even if without socket filter Make udp checksum be always available even if without socket filter.

Signed-off-by Wang Chen <[email protected]>

It checks the checksum in udp_recvmsg() and udp_poll().

Please do not fix bugs via code inspection without writing test programs to validate whether there is really a "bug" here or not.

author
#2 lore raw

David Miller said the following on 2007-11-16 10:54:

Quoted 8 lines

From: Wang Chen <[email protected]>
Date: Fri, 16 Nov 2007 10:46:26 +0800

Quoted 5 lines

The current kernel doesn't verify the udp checksum if user doesn't set a socket filter.It's fine for LAN. But for WAN, it's not a good option. Shall we fix it? Below is the patch to make udp checksum be always available.

Quoted 7 lines
Quoted 4 lines

[IPV4] UDP: Always checksum even if without socket filter Make udp checksum be always available even if without socket filter.

Signed-off-by Wang Chen <[email protected]>

It checks the checksum in udp_recvmsg() and udp_poll().

Actually, I tested and found this issue.
I sent 50 udp packets, which have wrong checksum, to my machine.
Before the patch being applied, /proc/net/snmp's udp InDatagrams increased, and InErrors didn't increase. After the patch being applied, InErrors increased correctly and InDatagrams stayed unchanging.

So, I think the checksum in udp_queue_rcv_skb() actually does the work, not that in udp_recvmsg() and udp_poll().

If I am wrong, please point out.

Quoted 4 lines

Please do not fix bugs via code inspection without writing test programs to validate whether there is really a "bug" here or not.

This is a bug from test, not from code inspection.

maintainer
#3 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 16 Nov 2007 11:18:16 +0800

Quoted 7 lines

Actually, I tested and found this issue.
I sent 50 udp packets, which have wrong checksum, to my machine.
Before the patch being applied, /proc/net/snmp's udp InDatagrams increased, and InErrors didn't increase. After the patch being applied, InErrors increased correctly and InDatagrams stayed unchanging.

That's right, InDataGrams will increase even if we haven't verified the checksum yet.

When the user does a recvmsg() or a poll() on the socket, we will notice the bad checksum then and increment InErrors. We could in this case correct the InDatagrams counter by decrementing it in this case.

author
#4 lore raw

David Miller said the following on 2007-11-16 12:04:

Quoted 11 lines

From: Wang Chen <[email protected]>
Date: Fri, 16 Nov 2007 11:18:16 +0800

That's right, InDataGrams will increase even if we haven't verified the checksum yet.

When the user does a recvmsg() or a poll() on the socket, we will notice the bad checksum then and increment InErrors. We could in this case correct the InDatagrams counter by decrementing it in this case.

Yes, it's a good idea.
I will make the patch and test it.
Thank dave for the suggestion.

#5 lore raw
In reply to David Miller
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"DM" == David Miller <[email protected]> writes:

DM> When the user does a recvmsg() or a poll() on the socket, we will DM> notice the bad checksum then and increment InErrors. We could in DM> this case correct the InDatagrams counter by decrementing it in DM> this case.

Does that mean that InDatagrams can decrease? I would expect some programs to interpret that as counter overflow.

/Benny

#6 lore raw
In reply to Wang Chen
Quoted 5 lines · Wang Chen

So, I think the checksum in udp_queue_rcv_skb() actually does the work, not that in udp_recvmsg() and udp_poll().

If I am wrong, please point out.

We may have a bug in the accounting area. Check the recent patch made to UDP/IPv6 which is probably needed here as well.

Hmm, we really need to spend more time on merging stuff between IPv4 and IPv6 to save all this duplication of effort.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#7 lore raw

Herbert Xu said the following on 2007-11-16 12:11:

Quoted 9 lines
Quoted 4 lines · Wang Chen

So, I think the checksum in udp_queue_rcv_skb() actually does the work, not that in udp_recvmsg() and udp_poll().

If I am wrong, please point out.

We may have a bug in the accounting area. Check the recent patch made to UDP/IPv6 which is probably needed here as well.

Like dave said, decrementing the InDataGrams in this case is an option.
I will check the same place of UDP/IPv6.

Quoted 3 lines

Hmm, we really need to spend more time on merging stuff between IPv4 and IPv6 to save all this duplication of effort.

Definitely necessary.

#8 lore raw
Quoted 7 lines · Wang Chen

Herbert Xu said the following on 2007-11-16 12:11:

Quoted 6 lines
Quoted 4 lines · Wang Chen

So, I think the checksum in udp_queue_rcv_skb() actually does the work, not that in udp_recvmsg() and udp_poll().

If I am wrong, please point out.

Quoted 7 lines
Quoted 3 lines

We may have a bug in the accounting area. Check the recent patch made to UDP/IPv6 which is probably needed here as well.

Like dave said, decrementing the InDataGrams in this case is an option.
I will check the same place of UDP/IPv6.

And like Benny pointed out it's probably a bad idea because decrementing counters will be an unexpected ABI change for monitoring programs who have no other way to detect overflow.

-Andi

maintainer
#9 lore raw

From: Andi Kleen <[email protected]>
Date: Sat, 17 Nov 2007 14:18:46 +0100

Quoted 13 lines
Quoted 11 lines · Wang Chen

Herbert Xu said the following on 2007-11-16 12:11:

Quoted 9 lines
Quoted 4 lines · Wang Chen

So, I think the checksum in udp_queue_rcv_skb() actually does the work, not that in udp_recvmsg() and udp_poll().

If I am wrong, please point out.

We may have a bug in the accounting area. Check the recent patch made to UDP/IPv6 which is probably needed here as well.

Quoted 7 lines
Quoted 3 lines

Like dave said, decrementing the InDataGrams in this case is an option.
I will check the same place of UDP/IPv6.

And like Benny pointed out it's probably a bad idea because decrementing counters will be an unexpected ABI change for monitoring programs who have no other way to detect overflow.

We could defer the increment until we check the checksum, but that is likely to break even more things because people (as Wang Chen did initially) will send a packet to some port with an app that doesn't eat the packets, and expect the InDatagrams counter to increase once the stack eats the packet. But it won't until the application does the read.

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue.

#10 lore raw
Quoted 5 lines

We could defer the increment until we check the checksum, but that is likely to break even more things because people (as Wang Chen did initially) will send a packet to some port with an app that doesn't eat the packets, and expect the InDatagrams counter to increase once the stack eats the packet.

Who expects that? Is there really any program who relies on that?

If it's just a human: there are a couple of "non intuitive" behaviours in the stack. This would be just another one. Not too big a deal.

Quoted 5 lines

But it won't until the application does the read.

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue.

If the SNMP monitor detects an false overflow the error it reports will be much worse than a single missing packet. So you would replace one error with a worse error.

-Andi

maintainer
#11 lore raw

From: Andi Kleen <[email protected]>
Date: Sun, 18 Nov 2007 22:45:15 +0100

Quoted 10 lines
Quoted 5 lines

We could defer the increment until we check the checksum, but that is likely to break even more things because people (as Wang Chen did initially) will send a packet to some port with an app that doesn't eat the packets, and expect the InDatagrams counter to increase once the stack eats the packet.

Who expects that? Is there really any program who relies on that?

If it's just a human: there are a couple of "non intuitive" behaviours in the stack. This would be just another one. Not too big a deal.

I would consider this a legitimate thing to check in a test suite such as TAHI or similar.

The networking stack DID receive the packet. Just because a socket owner is busy doing something else or blocked on some other event is no excuse not to bump the InDataGgrams counter.

The behavior would suck.

Quoted 9 lines
Quoted 5 lines

But it won't until the application does the read.

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue.

If the SNMP monitor detects an false overflow the error it reports will be much worse than a single missing packet. So you would replace one error with a worse error.

This can be fixed, the above cannot.

#12 lore raw
Quoted 4 lines · David Miller

The networking stack DID receive the packet. Just because a socket owner is busy doing something else or blocked on some other event is no excuse not to bump the InDataGgrams counter.

Actually if we ever implement the memory reclaim stuff for UDP when we're short on system memory then it could be possible for us to want to remove packets that are sitting on a socket's rcv queue. In that case it would be nice if InDataGrams wasn't incremented until the app took the packet off the socket.

In any case, I just looked up RFC1213 and it says:

udpInDatagrams OBJECT-TYPE
SYNTAX Counter
ACCESS read-only
STATUS mandatory
DESCRIPTION
"The total number of UDP datagrams delivered to UDP users."
::= { udp 1 }

So I think incrementing it in recvmsg is acceptable.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#13 lore raw

From: Herbert Xu <[email protected]>
Date: Mon, 19 Nov 2007 09:09:29 +0800

Quoted 11 lines

In any case, I just looked up RFC1213 and it says:

udpInDatagrams OBJECT-TYPE
SYNTAX Counter
ACCESS read-only
STATUS mandatory
DESCRIPTION
"The total number of UDP datagrams delivered to UDP users."
::= { udp 1 }

Quoted 1 line

So I think incrementing it in recvmsg is acceptable.

Point taken, thanks for checking that.

author
#14 lore raw
In reply to David Miller

David Miller said the following on 2007-11-19 6:40:

Quoted 5 lines

We could defer the increment until we check the checksum, but that is likely to break even more things because people (as Wang Chen did initially) will send a packet to some port with an app that doesn't eat the packets, and expect the InDatagrams counter to increase once the stack eats the packet.

As Herbert referred, RFC1213 says that udpInDatagrams records "The total number of UDP datagrams delivered to UDP users."

So if "udp_queue_rcv_skb() doing sucessfully" means "a UDP datagrams delivered to UDP users", the InDatagrams should be increased in udp_queue_rcv_skb().
Otherwise it should be increased until the UDP datagrams is actually delivered to UDP users.

maintainer
#15 lore raw

From: Wang Chen <[email protected]>
Date: Mon, 19 Nov 2007 10:02:03 +0800

Quoted 5 lines

So if "udp_queue_rcv_skb() doing sucessfully" means "a UDP datagrams delivered to UDP users", the InDatagrams should be increased in udp_queue_rcv_skb().
Otherwise it should be increased until the UDP datagrams is actually delivered to UDP users.

I agree.

#16 lore raw
In reply to David Miller
Quoted 2 lines · David Miller

This can be fixed, the above cannot.

That's a good point. Perhaps one way of getting that info to the user without putting it in UDPInDatagrams is to create an inet_diag interface for UDP and put the number of queued packets for each socket in it.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#17 lore raw

From: Herbert Xu <[email protected]>
Date: Mon, 19 Nov 2007 12:41:17 +0800

Quoted 8 lines · David Miller
Quoted 2 lines

This can be fixed, the above cannot.

That's a good point. Perhaps one way of getting that info to the user without putting it in UDPInDatagrams is to create an inet_diag interface for UDP and put the number of queued packets for each socket in it.

Right, if it's not what the RFC specifies, it's an auxiliary statistic we can provide some other way.

#18 lore raw
In reply to Herbert Xu

Em Mon, Nov 19, 2007 at 12:41:17PM +0800, Herbert Xu escreveu:

Quoted 8 lines · David Miller
Quoted 2 lines

This can be fixed, the above cannot.

That's a good point. Perhaps one way of getting that info to the user without putting it in UDPInDatagrams is to create an inet_diag interface for UDP and put the number of queued packets for each socket in it.

This is something I would love to see: UDP refactored so that it gets its hash tables/lookup code more and more looking like what we have in inet_hashtables (struct inet_hashinfo, etc) so that udp_diag would just fit in the inet_diag infrastructure, like dccp, that is in some aspects like TCP and in others like UDP and already uses this infrastructure.

- Arnaldo

#19 lore raw
In reply to David Miller
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue.

If the SNMP monitor detects an false overflow the error it reports will be much worse than a single missing packet. So you would replace one error with a worse error.

This can be fixed, the above cannot.

I don't see how, short of breaking the interface (e.g. reporting 64bit or separate overflow counts) -Andi

maintainer
#20 lore raw

From: Andi Kleen <[email protected]>
Date: Mon, 19 Nov 2007 16:29:33 +0100

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

If the SNMP monitor detects an false overflow the error it reports will be much worse than a single missing packet. So you would replace one error with a worse error.

This can be fixed, the above cannot.

I don't see how, short of breaking the interface (e.g. reporting 64bit or separate overflow counts)

As someone who just spent an entire weekend working on cpu performance counter code, I know it's possible.

When you overflow, the new value is "a lot" less than the last sampled one. When the value backtracks like we're discussing it could here, it only decrease a very little bit.

#21 lore raw
Quoted 17 lines · David Miller

From: Andi Kleen <[email protected]>
Date: Mon, 19 Nov 2007 16:29:33 +0100

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

If the SNMP monitor detects an false overflow the error it reports will be much worse than a single missing packet. So you would replace one error with a worse error.

This can be fixed, the above cannot.

I don't see how, short of breaking the interface (e.g. reporting 64bit or separate overflow counts)

Quoted 7 lines

As someone who just spent an entire weekend working on cpu performance counter code, I know it's possible.

When you overflow, the new value is "a lot" less than the last sampled one. When the value backtracks like we're discussing it could here, it only decrease a very little bit.

While I agree with your analysis that it could be worked around, who knows how all the various SNMP monitoring applications out there would interpret such an unusual event. I liked Stephen's suggestion of a deferred decrement that would insure the counter didn't ever run backwards. But the best approach seems to be just not to count it in the first place until tha application has actually received the packet, since as Herbert pointed out, that's what the RFC actually specifies for the meaning of the udpInDatagrams counter.

-Bill

maintainer
#22 lore raw

From: Bill Fink <[email protected]>
Date: Tue, 20 Nov 2007 00:29:45 -0500

Quoted 4 lines

But the best approach seems to be just not to count
it in the first place until tha application has actually received the packet, since as Herbert pointed out, that's what the RFC actually specifies for the meaning of the udpInDatagrams counter.

I agree and would be happy to apply such a patch :)

author
#23 lore raw

David Miller said the following on 2007-11-20 14:15:

Quoted 12 lines

From: Bill Fink <[email protected]>
Date: Tue, 20 Nov 2007 00:29:45 -0500

Quoted 4 lines

But the best approach seems to be just not to count
it in the first place until tha application has actually received the packet, since as Herbert pointed out, that's what the RFC actually specifies for the meaning of the udpInDatagrams counter.

I agree and would be happy to apply such a patch :)

I want to wait for more suggestions until make such patch. Because this solution leads to troubles with some apps, such as NFS.
(http://bugzilla.kernel.org/show_bug.cgi?id=6660#c2)

#24 lore raw
Quoted 4 lines · Wang Chen

I want to wait for more suggestions until make such patch. Because this solution leads to troubles with some apps, such as NFS.
(http://bugzilla.kernel.org/show_bug.cgi?id=6660#c2)

Well that's easy. Just get NFS to increment the counters itself. Of course if someone could merge that code so that NFS uses more generic code then it's even better.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#25 lore raw
In reply to Bill Fink
Quoted 8 lines · Bill Fink

While I agree with your analysis that it could be worked around, who knows how all the various SNMP monitoring applications out there would interpret such an unusual event. I liked Stephen's suggestion of a deferred decrement that would insure the counter didn't ever run backwards. But the best approach seems to be just not to count it in the first place until tha application has actually received the packet, since as Herbert pointed out, that's what the RFC actually specifies for the meaning of the udpInDatagrams counter.

Together with another counter that counts "edge datagrams received" that would be an excellent idea.

Here's a patch.

-Andi

---

Split UDP receive count into UdpInDatagrams and UdpInEarlyDatagrams

UdpInDatagrams can be confusing because it counts packets that might be dropped later.

Move UdpInDatagrams into recvmsg() as allowed by the RFC.

Add a new UdpInEarlyDatagrams counter to count datagrams received early, but which might be dropped later

Signed-off-by Andi Kleen <[email protected]>
include/linux/snmp.h +1 -0
Index: linux-2.6.24-rc1-hack/include/linux/snmp.h
===================================================================
@@ -138,6 +138,7 @@ enum
 	UDP_MIB_OUTDATAGRAMS,			/* OutDatagrams */
 	UDP_MIB_RCVBUFERRORS,			/* RcvbufErrors */
 	UDP_MIB_SNDBUFERRORS,			/* SndbufErrors */
+	UDP_MIB_INEARLYDATAGRAMS,		/* Early Datagrams Received */
 	__UDP_MIB_MAX
 };
net/ipv4/udp.c +5 -2
Index: linux-2.6.24-rc1-hack/net/ipv4/udp.c
===================================================================
@@ -873,6 +873,8 @@ try_again:
 	if (err)
 		goto out_free;
 
+	UDP_INC_STATS_USER(UDP_MIB_INDATAGRAMS, is_udplite);
+
 	sock_recv_timestamp(msg, sk, skb);
 
 	/* Copy the address. */

@@ -967,7 +969,8 @@ int udp_queue_rcv_skb(struct sock * sk, 
 
 			ret = (*up->encap_rcv)(sk, skb);
 			if (ret <= 0) {
-				UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS, up->pcflag);
+				UDP_INC_STATS_BH(UDP_MIB_INEARLYDATAGRAMS,
+						 up->pcflag);
 				return -ret;
 			}
 		}

@@ -1023,7 +1026,7 @@ int udp_queue_rcv_skb(struct sock * sk, 
 		goto drop;
 	}
 
-	UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS, up->pcflag);
+	UDP_INC_STATS_BH(UDP_MIB_INEARLYDATAGRAMS, up->pcflag);
 	return 0;
 
 drop:
net/ipv4/proc.c +1 -0
Index: linux-2.6.24-rc1-hack/net/ipv4/proc.c
===================================================================
@@ -173,6 +173,7 @@ static const struct snmp_mib snmp4_udp_l
 	SNMP_MIB_ITEM("OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("InEarlyDatagrams", UDP_MIB_INEARLYDATAGRAMS),
 	SNMP_MIB_SENTINEL
 };
maintainer
#26 lore raw

From: Andi Kleen <[email protected]>
Date: Tue, 20 Nov 2007 15:05:18 +0100

Quoted 10 lines · Bill Fink
Quoted 8 lines

While I agree with your analysis that it could be worked around, who knows how all the various SNMP monitoring applications out there would interpret such an unusual event. I liked Stephen's suggestion of a deferred decrement that would insure the counter didn't ever run backwards. But the best approach seems to be just not to count it in the first place until tha application has actually received the packet, since as Herbert pointed out, that's what the RFC actually specifies for the meaning of the udpInDatagrams counter.

Quoted 4 lines

Together with another counter that counts "edge datagrams received" that would be an excellent idea.

Here's a patch.

NFS and friends that use the ->data_ready() callback needs to be updated as well. Please fix this and resubmit, thanks.

author
#27 lore raw

David Miller said the following on 2007-11-21 9:39:

Quoted 12 lines

From: Andi Kleen <[email protected]>
Date: Tue, 20 Nov 2007 15:05:18 +0100

Quoted 9 lines · Bill Fink
Quoted 8 lines

While I agree with your analysis that it could be worked around, who knows how all the various SNMP monitoring applications out there would interpret such an unusual event. I liked Stephen's suggestion of a deferred decrement that would insure the counter didn't ever run backwards. But the best approach seems to be just not to count it in the first place until tha application has actually received the packet, since as Herbert pointed out, that's what the RFC actually specifies for the meaning of the udpInDatagrams counter.

Quoted 8 lines
Quoted 4 lines

Together with another counter that counts "edge datagrams received" that would be an excellent idea.

Here's a patch.

NFS and friends that use the ->data_ready() callback needs to be updated as well. Please fix this and resubmit, thanks.

I tested nfsv3 & nfsv4. It seems that nfs calls recvmsg() like following:nfsd()->svc_recv()->svc_udp_recvfrom()->udp_recvmsg(). So, I think putting the udpInDatagrams increment in udp_recvmsg() is enough.

FYI:
http://www.mail-archive.com/[email protected]/msg13817.html

#28 lore raw
Quoted 8 lines · Wang Chen

I tested nfsv3 & nfsv4. It seems that nfs calls recvmsg() like following:nfsd()->svc_recv()->svc_udp_recvfrom()->udp_recvmsg(). So, I think putting the udpInDatagrams increment in udp_recvmsg() is enough.

FYI:
http://www.mail-archive.com/[email protected]/msg13817.html

Excellent. They now do a recvmsg first with no buffer to get meta-information, which just happens to increment the counters.

Could you please resubmit the patch then?

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#29 lore raw

Herbert Xu said the following on 2007-11-29 17:21:

Quoted 7 lines · Wang Chen

Excellent. They now do a recvmsg first with no buffer to get meta-information, which just happens to increment the counters.

Could you please resubmit the patch then?

[SNMP]: Defer InDataGrams increment until recvmsg() does checksum

Split UDP receive count into UdpInDatagrams and UdpInEarlyDatagrams

UdpInDatagrams can be confusing because it counts packets that might be dropped later.

Move UdpInDatagrams into recvmsg() as allowed by the RFC.

Add a new UdpInEarlyDatagrams counter to count datagrams received early, but which might be dropped later.

Signed-off-by Andi Kleen <[email protected]>
Signed-off-by Wang Chen <[email protected]>
---
 Documentation/networking/udplite.txt |    2 +-
 include/linux/snmp.h                 |    1 +
 net/ipv4/proc.c                      |    1 +
 net/ipv4/udp.c                       |   12 ++++++++----
 net/ipv6/proc.c                      |    1 +
 net/ipv6/udp.c                       |   13 ++++++++-----
 6 files changed, 20 insertions(+), 10 deletions(-)
Documentation/networking/udplite.txt +1 -1
diff -Nurp linux-2.6.24.rc3.org/Documentation/networking/udplite.txt linux-2.6.24.rc3/Documentation/networking/udplite.txt
@@ -236,7 +236,7 @@
 
   This displays UDP-Lite statistics variables, whose meaning is as follows.
 
-   InDatagrams:     Total number of received datagrams.
+   InDatagrams:     The total number of UDP datagrams delivered to UDP users.
 
    NoPorts:         Number of packets received to an unknown port.
                     These cases are counted separately (not as InErrors).
include/linux/snmp.h +1 -0
diff -Nurp linux-2.6.24.rc3.org/include/linux/snmp.h linux-2.6.24.rc3/include/linux/snmp.h
@@ -138,6 +138,7 @@ enum
 	UDP_MIB_OUTDATAGRAMS,			/* OutDatagrams */
 	UDP_MIB_RCVBUFERRORS,			/* RcvbufErrors */
 	UDP_MIB_SNDBUFERRORS,			/* SndbufErrors */
+	UDP_MIB_INEARLYDATAGRAMS,		/* Early Datagrams Received */
 	__UDP_MIB_MAX
 };
net/ipv4/proc.c +1 -0
diff -Nurp linux-2.6.24.rc3.org/net/ipv4/proc.c linux-2.6.24.rc3/net/ipv4/proc.c
@@ -149,6 +149,7 @@ static const struct snmp_mib snmp4_tcp_l
 
 static const struct snmp_mib snmp4_udp_list[] = {
 	SNMP_MIB_ITEM("InDatagrams", UDP_MIB_INDATAGRAMS),
+	SNMP_MIB_ITEM("InEarlyDatagrams", UDP_MIB_INEARLYDATAGRAMS),
 	SNMP_MIB_ITEM("NoPorts", UDP_MIB_NOPORTS),
 	SNMP_MIB_ITEM("InErrors", UDP_MIB_INERRORS),
 	SNMP_MIB_ITEM("OutDatagrams", UDP_MIB_OUTDATAGRAMS),
net/ipv4/udp.c +8 -4
diff -Nurp linux-2.6.24.rc3.org/net/ipv4/udp.c linux-2.6.24.rc3/net/ipv4/udp.c
@@ -873,6 +873,8 @@ try_again:
 	if (err)
 		goto out_free;
 
+	UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS, is_udplite);
+
 	sock_recv_timestamp(msg, sk, skb);
 
 	/* Copy the address. */

@@ -940,6 +942,7 @@ int udp_queue_rcv_skb(struct sock * sk, 
 {
 	struct udp_sock *up = udp_sk(sk);
 	int rc;
+	int is_udplite = IS_UDPLITE(sk);
 
 	/*
 	 *	Charge it to the socket, dropping if the queue is full.

@@ -967,7 +970,8 @@ int udp_queue_rcv_skb(struct sock * sk, 
 
 			ret = (*up->encap_rcv)(sk, skb);
 			if (ret <= 0) {
-				UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS, up->pcflag);
+				UDP_INC_STATS_BH(UDP_MIB_INEARLYDATAGRAMS,
+						 is_udplite);
 				return -ret;
 			}
 		}

@@ -1019,15 +1023,15 @@ int udp_queue_rcv_skb(struct sock * sk, 
 	if ((rc = sock_queue_rcv_skb(sk,skb)) < 0) {
 		/* Note that an ENOMEM error is charged twice */
 		if (rc == -ENOMEM)
-			UDP_INC_STATS_BH(UDP_MIB_RCVBUFERRORS, up->pcflag);
+			UDP_INC_STATS_BH(UDP_MIB_RCVBUFERRORS, is_udplite);
 		goto drop;
 	}
 
-	UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS, up->pcflag);
+	UDP_INC_STATS_BH(UDP_MIB_INEARLYDATAGRAMS, is_udplite);
 	return 0;
 
 drop:
-	UDP_INC_STATS_BH(UDP_MIB_INERRORS, up->pcflag);
+	UDP_INC_STATS_BH(UDP_MIB_INERRORS, is_udplite);
 	kfree_skb(skb);
 	return -1;
 }
net/ipv6/proc.c +1 -0
diff -Nurp linux-2.6.24.rc3.org/net/ipv6/proc.c linux-2.6.24.rc3/net/ipv6/proc.c
@@ -104,6 +104,7 @@ static char *icmp6type2name[256] = {
 
 static struct snmp_mib snmp6_udp6_list[] = {
 	SNMP_MIB_ITEM("Udp6InDatagrams", UDP_MIB_INDATAGRAMS),
+	SNMP_MIB_ITEM("Udp6InEarlyDatagrams", UDP_MIB_INEARLYDATAGRAMS),
 	SNMP_MIB_ITEM("Udp6NoPorts", UDP_MIB_NOPORTS),
 	SNMP_MIB_ITEM("Udp6InErrors", UDP_MIB_INERRORS),
 	SNMP_MIB_ITEM("Udp6OutDatagrams", UDP_MIB_OUTDATAGRAMS),
net/ipv6/udp.c +8 -5
diff -Nurp linux-2.6.24.rc3.org/net/ipv6/udp.c linux-2.6.24.rc3/net/ipv6/udp.c
@@ -164,6 +164,8 @@ try_again:
 	if (err)
 		goto out_free;
 
+	UDP6_INC_STATS_BH(UDP_MIB_INDATAGRAMS, is_udplite);
+
 	sock_recv_timestamp(msg, sk, skb);
 
 	/* Copy the address. */

@@ -205,7 +207,7 @@ out:
 	return err;
 
 csum_copy_err:
-	UDP6_INC_STATS_USER(UDP_MIB_INERRORS, is_udplite);
+	UDP6_INC_STATS_BH(UDP_MIB_INERRORS, is_udplite);
 	skb_kill_datagram(sk, skb, flags);
 
 	if (flags & MSG_DONTWAIT)

@@ -258,6 +260,7 @@ int udpv6_queue_rcv_skb(struct sock * sk
 {
 	struct udp_sock *up = udp_sk(sk);
 	int rc;
+	int is_udplite = IS_UDPLITE(sk);
 
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto drop;

@@ -265,7 +268,7 @@ int udpv6_queue_rcv_skb(struct sock * sk
 	/*
 	 * UDP-Lite specific tests, ignored on UDP sockets (see net/ipv4/udp.c).
 	 */
-	if ((up->pcflag & UDPLITE_RECV_CC)  &&  UDP_SKB_CB(skb)->partial_cov) {
+	if ((is_udplite & UDPLITE_RECV_CC)  &&  UDP_SKB_CB(skb)->partial_cov) {
 
 		if (up->pcrlen == 0) {          /* full coverage was set  */
 			LIMIT_NETDEBUG(KERN_WARNING "UDPLITE6: partial coverage"

@@ -289,13 +292,13 @@ int udpv6_queue_rcv_skb(struct sock * sk
 	if ((rc = sock_queue_rcv_skb(sk,skb)) < 0) {
 		/* Note that an ENOMEM error is charged twice */
 		if (rc == -ENOMEM)
-			UDP6_INC_STATS_BH(UDP_MIB_RCVBUFERRORS, up->pcflag);
+			UDP6_INC_STATS_BH(UDP_MIB_RCVBUFERRORS, is_udplite);
 		goto drop;
 	}
-	UDP6_INC_STATS_BH(UDP_MIB_INDATAGRAMS, up->pcflag);
+	UDP6_INC_STATS_BH(UDP_MIB_INEARLYDATAGRAMS, is_udplite);
 	return 0;
 drop:
-	UDP6_INC_STATS_BH(UDP_MIB_INERRORS, up->pcflag);
+	UDP6_INC_STATS_BH(UDP_MIB_INERRORS, is_udplite);
 	kfree_skb(skb);
 	return -1;
 }
#30 lore raw
Quoted 3 lines · Wang Chen

Add a new UdpInEarlyDatagrams counter to count datagrams received early, but which might be dropped later.

Could you please split this into two patches? Have one do the UdpInDatagrams change and the other to introduce the EarlyDatagrams counter.

I'm a bit hesitant to introduce new counters in the MIB because it'd be difficult if not impossible to ever remove them. Do you really need the early counter?

One more thing, please put the is_udplite clean-up in its own patch too so it's absolutely clear what we're changing in the patches that aren't clean-ups.

Quoted 2 lines
Signed-off-by Andi Kleen <[email protected]>
Signed-off-by Wang Chen <[email protected]>

Who's the author? Andi or you? Please make this obvious with a From header when you resubmit.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#31 lore raw

Herbert Xu said the following on 2007-11-29 18:21:

Quoted 12 lines · Wang Chen
Quoted 2 lines

Add a new UdpInEarlyDatagrams counter to count datagrams received early, but which might be dropped later.

Could you please split this into two patches? Have one do the UdpInDatagrams change and the other to introduce the EarlyDatagrams counter.

I'm a bit hesitant to introduce new counters in the MIB because it'd be difficult if not impossible to ever remove them. Do you really need the early counter?

I cooked the patch based on Andi's and left the new counter. Frankly, I don't like the EarlyDatagrams too.
So, I will remove it and resubmit.

Quoted 4 lines

One more thing, please put the is_udplite clean-up in its own patch too so it's absolutely clear what we're changing in the patches that aren't clean-ups.

OK.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Andi Kleen <[email protected]>
Signed-off-by Wang Chen <[email protected]>

Who's the author? Andi or you? Please make this obvious with a From header when you resubmit.

Since I will remove the new counter idea of Andi, there will be only one author. :)

#32 lore raw
Quoted 4 lines · Wang Chen

I cooked the patch based on Andi's and left the new counter. Frankly, I don't like the EarlyDatagrams too.
So, I will remove it and resubmit.

Sounds good. Thanks for all your efforts on this problem!

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#33 lore raw
In reply to Wang Chen

Thank you for doing this work, there is a small comment below.

| --- linux-2.6.24.rc3.org/Documentation/networking/udplite.txt	2007-11-19 12:37:40.000000000 +0800
| +++ linux-2.6.24.rc3/Documentation/networking/udplite.txt	2007-11-28 18:35:29.000000000 +0800
| @@ -236,7 +236,7 @@
|  
|    This displays UDP-Lite statistics variables, whose meaning is as follows.
|  
| -   InDatagrams:     Total number of received datagrams.
| +   InDatagrams:     The total number of UDP datagrams delivered to UDP users.

You are in the UDP-Lite documentation -- it should read "UDP-Lite", not UDP.

#34 lore raw
Quoted 4 lines · Gerrit Renker
| -   InDatagrams:     Total number of received datagrams.
| +   InDatagrams:     The total number of UDP datagrams delivered to UDP users.

You are in the UDP-Lite documentation -- it should read "UDP-Lite", not UDP.

We could just drop the mention of UDP completely:

The total number of datagrams delivered to applications.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#35 lore raw
In reply to David Miller
Quoted 35 lines · David Miller

From: Andi Kleen <[email protected]>
Date: Sat, 17 Nov 2007 14:18:46 +0100

Quoted 16 lines
Quoted 13 lines · Wang Chen

Herbert Xu said the following on 2007-11-16 12:11:

Quoted 11 lines
Quoted 5 lines · Wang Chen

So, I think the checksum in udp_queue_rcv_skb() actually does the work, not that in udp_recvmsg() and udp_poll().

If I am wrong, please point out.

We may have a bug in the accounting area. Check the recent patch made to UDP/IPv6 which is probably needed here as well.

Quoted 7 lines
Quoted 4 lines

Like dave said, decrementing the InDataGrams in this case is an option.
I will check the same place of UDP/IPv6.

And like Benny pointed out it's probably a bad idea because
decrementing counters will be an unexpected ABI change for monitoring programs who have no other way to detect overflow.

We could defer the increment until we check the checksum, but that is likely to break even more things because people (as Wang Chen did initially) will send a packet to some port with an app that doesn't eat the packets, and expect the InDatagrams counter to increase once the stack eats the packet. But it won't until the application does the read.

Quoted 5 lines

All of our options suck, we just have to choose the least sucking one and right now to me that's decrementing the counter as much as I empathize with the SNMP application overflow detection issue. More majordomo info at http://vger.kernel.org/majordomo-info.html

You might be able hide the problem by adding some "defered tick logic" so that the counter is monotonic. Kind of like the NTP clock holding hack.