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

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

[Wang Chen]

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 <wangchen@cn.fujitsu.com>
---
 udp.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

--- linux-2.6.24-rc2.org/net/ipv4/udp.c	2007-11-09 16:37:57.000000000 +0800
+++ linux-2.6.24-rc2/net/ipv4/udp.c	2007-11-16 10:10:35.000000000 +0800
@@ -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 */

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 16 Nov 2007 10:46:26 +0800

> 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 <wangchen@cn.fujitsu.com>

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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

David Miller said the following on 2007-11-16 10:54:
> From: Wang Chen <wangchen@cn.fujitsu.com>
> Date: Fri, 16 Nov 2007 10:46:26 +0800
> 
>> 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 <wangchen@cn.fujitsu.com>
> 
> 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.

> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 16 Nov 2007 11:18:16 +0800

> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

Wang Chen <wangchen@cn.fujitsu.com> wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

David Miller said the following on 2007-11-16 12:04:
> From: Wang Chen <wangchen@cn.fujitsu.com>
> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

Herbert Xu said the following on 2007-11-16 12:11:
> Wang Chen <wangchen@cn.fujitsu.com> wrote:
>> 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.

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

Definitely necessary.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Benny Amorsen]

>>>>> "DM" == David Miller <davem@davemloft.net> 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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Andi Kleen]

Wang Chen <wangchen@cn.fujitsu.com> writes:

> Herbert Xu said the following on 2007-11-16 12:11:
>> Wang Chen <wangchen@cn.fujitsu.com> wrote:
>>> 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.

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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Sat, 17 Nov 2007 14:18:46 +0100

> Wang Chen <wangchen@cn.fujitsu.com> writes:
> 
> > Herbert Xu said the following on 2007-11-16 12:11:
> >> Wang Chen <wangchen@cn.fujitsu.com> wrote:
> >>> 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.
> 
> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Andi Kleen]

> 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.

> 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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Stephen Hemminger]

David Miller wrote:
> From: Andi Kleen <andi@firstfloor.org>
> Date: Sat, 17 Nov 2007 14:18:46 +0100
>
>   
>> Wang Chen <wangchen@cn.fujitsu.com> writes:
>>
>>     
>>> Herbert Xu said the following on 2007-11-16 12:11:
>>>       
>>>> Wang Chen <wangchen@cn.fujitsu.com> wrote:
>>>>         
>>>>> 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.
>>>       
>> 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.
> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Sun, 18 Nov 2007 22:45:15 +0100

> > 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.

> > 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Sun, Nov 18, 2007 at 02:40:10PM -0800, David Miller wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

David Miller said the following on 2007-11-19 6:40:
> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Sun, Nov 18, 2007 at 02:40:10PM -0800, David Miller wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 19 Nov 2007 09:09:29 +0800

> 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.

Point taken, thanks for checking that.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 19 Nov 2007 10:02:03 +0800

> 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 19 Nov 2007 12:41:17 +0800

> On Sun, Nov 18, 2007 at 02:40:10PM -0800, David Miller wrote:
> >
> > 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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Arnaldo Carvalho de Melo]

Em Mon, Nov 19, 2007 at 12:41:17PM +0800, Herbert Xu escreveu:
> On Sun, Nov 18, 2007 at 02:40:10PM -0800, David Miller wrote:
> >
> > 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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Andi Kleen]

> > > 
> > > 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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Mon, 19 Nov 2007 16:29:33 +0100

> > > > 
> > > > 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)

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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Bill Fink]

On Mon, 19 Nov 2007, David Miller wrote:

> From: Andi Kleen <andi@firstfloor.org>
> Date: Mon, 19 Nov 2007 16:29:33 +0100
> 
> > > > > 
> > > > > 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)
> 
> 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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Bill Fink <billfink@mindspring.com>
Date: Tue, 20 Nov 2007 00:29:45 -0500

> 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 :)

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

David Miller said the following on 2007-11-20 14:15:
> From: Bill Fink <billfink@mindspring.com>
> Date: Tue, 20 Nov 2007 00:29:45 -0500
> 
>> 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)

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Tue, Nov 20, 2007 at 02:25:23PM +0800, Wang Chen wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Andi Kleen]

On Tue, Nov 20, 2007 at 12:29:45AM -0500, Bill Fink wrote:
> 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 <ak@suse.de>

Index: linux-2.6.24-rc1-hack/include/linux/snmp.h
===================================================================
--- linux-2.6.24-rc1-hack.orig/include/linux/snmp.h
+++ 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
 };
 
Index: linux-2.6.24-rc1-hack/net/ipv4/udp.c
===================================================================
--- linux-2.6.24-rc1-hack.orig/net/ipv4/udp.c
+++ 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:
Index: linux-2.6.24-rc1-hack/net/ipv4/proc.c
===================================================================
--- linux-2.6.24-rc1-hack.orig/net/ipv4/proc.c
+++ 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
 };
 

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Tue, 20 Nov 2007 15:05:18 +0100

> On Tue, Nov 20, 2007 at 12:29:45AM -0500, Bill Fink wrote:
> > 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.

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

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

David Miller said the following on 2007-11-21 9:39:
> From: Andi Kleen <andi@firstfloor.org>
> Date: Tue, 20 Nov 2007 15:05:18 +0100
> 
>> On Tue, Nov 20, 2007 at 12:29:45AM -0500, Bill Fink wrote:
>>> 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.
> 
> 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/netdev@vger.kernel.org/msg13817.html

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Thu, Nov 29, 2007 at 03:55:38PM +0800, Wang Chen wrote:
> 
> 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/netdev@vger.kernel.org/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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

Herbert Xu said the following on 2007-11-29 17:21:
> On Thu, Nov 29, 2007 at 03:55:38PM +0800, Wang Chen wrote:
> 
> 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 <ak@suse.de>
Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 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(-)

diff -Nurp linux-2.6.24.rc3.org/Documentation/networking/udplite.txt linux-2.6.24.rc3/Documentation/networking/udplite.txt
--- 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.
 
    NoPorts:         Number of packets received to an unknown port.
                     These cases are counted separately (not as InErrors).
diff -Nurp linux-2.6.24.rc3.org/include/linux/snmp.h linux-2.6.24.rc3/include/linux/snmp.h
--- linux-2.6.24.rc3.org/include/linux/snmp.h	2007-11-19 12:38:13.000000000 +0800
+++ linux-2.6.24.rc3/include/linux/snmp.h	2007-11-28 18:06:15.000000000 +0800
@@ -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
 };
 
diff -Nurp linux-2.6.24.rc3.org/net/ipv4/proc.c linux-2.6.24.rc3/net/ipv4/proc.c
--- linux-2.6.24.rc3.org/net/ipv4/proc.c	2007-11-19 12:38:14.000000000 +0800
+++ linux-2.6.24.rc3/net/ipv4/proc.c	2007-11-28 18:06:15.000000000 +0800
@@ -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),
diff -Nurp linux-2.6.24.rc3.org/net/ipv4/udp.c linux-2.6.24.rc3/net/ipv4/udp.c
--- linux-2.6.24.rc3.org/net/ipv4/udp.c	2007-11-19 12:38:14.000000000 +0800
+++ linux-2.6.24.rc3/net/ipv4/udp.c	2007-11-29 17:24:25.000000000 +0800
@@ -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;
 }
diff -Nurp linux-2.6.24.rc3.org/net/ipv6/proc.c linux-2.6.24.rc3/net/ipv6/proc.c
--- linux-2.6.24.rc3.org/net/ipv6/proc.c	2007-11-19 12:38:14.000000000 +0800
+++ linux-2.6.24.rc3/net/ipv6/proc.c	2007-11-28 18:06:15.000000000 +0800
@@ -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),
diff -Nurp linux-2.6.24.rc3.org/net/ipv6/udp.c linux-2.6.24.rc3/net/ipv6/udp.c
--- linux-2.6.24.rc3.org/net/ipv6/udp.c	2007-11-19 12:38:14.000000000 +0800
+++ linux-2.6.24.rc3/net/ipv6/udp.c	2007-11-29 17:25:11.000000000 +0800
@@ -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;
 }

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Thu, Nov 29, 2007 at 06:08:30PM +0800, Wang Chen wrote:
> 
> 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.

> Signed-off-by: Andi Kleen <ak@suse.de>
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

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

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Wang Chen]

Herbert Xu said the following on 2007-11-29 18:21:
> On Thu, Nov 29, 2007 at 06:08:30PM +0800, Wang Chen wrote:
>> 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.

> 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.

>> Signed-off-by: Andi Kleen <ak@suse.de>
>> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> 
> 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. :)

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Thu, Nov 29, 2007 at 06:33:01PM +0800, Wang Chen wrote:
>
> 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!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Gerrit Renker]

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.

Re: [PATCH 1/2] [IPV4] UDP: Always checksum even if without socket filter

[Herbert Xu]

On Thu, Nov 29, 2007 at 10:56:48AM +0000, Gerrit Renker wrote:
>
> | -   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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt