Bug 17622 - snmp trap ALG issue
Summary: snmp trap ALG issue
Status: RESOLVED OBSOLETE
Alias: None
Product: Networking
Classification: Unclassified
Component: IPV4 (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Stephen Hemminger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-02 09:25 UTC by clark wang
Modified: 2012-08-13 16:09 UTC (History)
2 users (show)

See Also:
Kernel Version: 2.6.35.4
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description clark wang 2010-09-02 09:25:11 UTC
Symptom:
SNMP manager can't show trap when SNMP agent set trap message to  version1.

steps to reproduce:
(1)SNMP agent-----linux device(NAT)-----SNMP manager.
(2)Set SNMP agent trap message to version 1, destination IP as SNMP   manger's IP.
(3)Do some operation to generate trap message, such as make one port of SNMP agent up and down. But SNMP manger can't accept trap message.

I have capured the packet by Ethereal software, and check the SNMP trap packet, found that the UDP checksum is incorrect. 
I think that the function fast_csum()(nf_nat_snmp_basic.c) have some problem.
Comment 1 clark wang 2010-09-02 09:39:03 UTC
I have changed this function by refering to other checksum algorithm.
And tested it, it can work. the checksum is correct. 

static void fast_csum(__sum16 *csum,
		      const unsigned char *optr,
		      const unsigned char *nptr,
		      int offset)
{
	unsigned char s[4];

	if (offset & 1) {
		s[0] = s[2] = 0;
		s[0] = ~s[0]; //this line is add by me
		s[1] = ~*optr;
		s[3] = *nptr;
	} else {
		s[1] = s[3] = 0;
		s[1] = ~s[1];//this line is add by me
		s[0] = ~*optr;
		s[2] = *nptr;
	}

	*csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
}
Comment 2 Andrew Morton 2010-09-14 23:32:38 UTC
(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Thu, 2 Sep 2010 09:25:12 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:

> https://bugzilla.kernel.org/show_bug.cgi?id=17622
> 
>            Summary: snmp trap ALG issue
>            Product: Networking
>            Version: 2.5
>     Kernel Version: 2.6.35.4
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: IPV4
>         AssignedTo: shemminger@linux-foundation.org
>         ReportedBy: wtweeker@163.com
>         Regression: No
> 
> 
> Symptom:
> SNMP manager can't show trap when SNMP agent set trap message to  version1.
> 
> steps to reproduce:
> (1)SNMP agent-----linux device(NAT)-----SNMP manager.
> (2)Set SNMP agent trap message to version 1, destination IP as SNMP  
> manger's
> IP.
> (3)Do some operation to generate trap message, such as make one port of SNMP
> agent up and down. But SNMP manger can't accept trap message.
> 
> I have capured the packet by Ethereal software, and check the SNMP trap
> packet,
> found that the UDP checksum is incorrect. 
> I think that the function fast_csum()(nf_nat_snmp_basic.c) have some problem.
> 

and


> I have changed this function by refering to other checksum algorithm.
> And tested it, it can work. the checksum is correct. 
> 
> static void fast_csum(__sum16 *csum,
>               const unsigned char *optr,
>               const unsigned char *nptr,
>               int offset)
> {
>     unsigned char s[4];
> 
>     if (offset & 1) {
>         s[0] = s[2] = 0;
>         s[0] = ~s[0]; //this line is add by me
>         s[1] = ~*optr;
>         s[3] = *nptr;
>     } else {
>         s[1] = s[3] = 0;
>         s[1] = ~s[1];//this line is add by me
>         s[0] = ~*optr;
>         s[2] = *nptr;
>     }
> 
>     *csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
> }

Great.  Please prepare a kernel patch as per
Documentation/SubmittingPatches and send it via a reply-to-all to this
email?

Thanks.
Comment 3 clark wang 2010-09-17 03:35:40 UTC
Dear All

                  The attachment is the patch of this bug.

Thanks




At 2010-09-15 07:32:08,"Andrew Morton" <akpm@linux-foundation.org> wrote:

>(switched to email.  Please respond via emailed reply-to-all, not via the
>bugzilla web interface).
>
>On Thu, 2 Sep 2010 09:25:12 GMT
>bugzilla-daemon@bugzilla.kernel.org wrote:
>
>> https://bugzilla.kernel.org/show_bug.cgi?id=17622
>> 
>>            Summary: snmp trap ALG issue
>>            Product: Networking
>>            Version: 2.5
>>     Kernel Version: 2.6.35.4
>>           Platform: All
>>         OS/Version: Linux
>>               Tree: Mainline
>>             Status: NEW
>>           Severity: normal
>>           Priority: P1
>>          Component: IPV4
>>         AssignedTo: shemminger@linux-foundation.org
>>         ReportedBy: wtweeker@163.com
>>         Regression: No
>> 
>> 
>> Symptom:
>> SNMP manager can't show trap when SNMP agent set trap message to  version1.
>> 
>> steps to reproduce:
>> (1)SNMP agent-----linux device(NAT)-----SNMP manager.
>> (2)Set SNMP agent trap message to version 1, destination IP as SNMP  
>> manger's
>> IP.
>> (3)Do some operation to generate trap message, such as make one port of SNMP
>> agent up and down. But SNMP manger can't accept trap message.
>> 
>> I have capured the packet by Ethereal software, and check the SNMP trap
>> packet,
>> found that the UDP checksum is incorrect. 
>> I think that the function fast_csum()(nf_nat_snmp_basic.c) have some
>> problem.
>> 
>
>and
>
>
>> I have changed this function by refering to other checksum algorithm.
>> And tested it, it can work. the checksum is correct. 
>> 
>> static void fast_csum(__sum16 *csum,
>>               const unsigned char *optr,
>>               const unsigned char *nptr,
>>               int offset)
>> {
>>     unsigned char s[4];
>> 
>>     if (offset & 1) {
>>         s[0] = s[2] = 0;
>>         s[0] = ~s[0]; //this line is add by me
>>         s[1] = ~*optr;
>>         s[3] = *nptr;
>>     } else {
>>         s[1] = s[3] = 0;
>>         s[1] = ~s[1];//this line is add by me
>>         s[0] = ~*optr;
>>         s[2] = *nptr;
>>     }
>> 
>>     *csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
>> }
>
>Great.  Please prepare a kernel patch as per
>Documentation/SubmittingPatches and send it via a reply-to-all to this
>email?
>
>Thanks.
Comment 4 Anonymous Emailer 2010-09-17 06:35:57 UTC
Reply-To: shemminger@vyatta.com

I think the bug should be fixed by removing the potentially buggy
fast_csum() in nf_nat_snmp_basic and just using the existing
generic code. The following is compile tested only..

Subject: [PATCH] nf_nat_snmp: use existing checksum update code

The fast_csum() in NAT code for processing SNMP trap is buggy 
(see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
Replace it by using the existing checksum replacement code;
it means adding a new csum_replace1() inline wrapper.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 include/net/checksum.h                 |    5 +++++
 net/ipv4/netfilter/nf_nat_snmp_basic.c |   31 ++-----------------------------
 2 files changed, 7 insertions(+), 29 deletions(-)

--- a/net/ipv4/netfilter/nf_nat_snmp_basic.c	2010-09-16 22:17:21.660806917 -0700
+++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c	2010-09-16 22:32:52.084075112 -0700
@@ -882,30 +882,6 @@ static unsigned char snmp_request_decode
 }
 
 /*
- * Fast checksum update for possibly oddly-aligned UDP byte, from the
- * code example in the draft.
- */
-static void fast_csum(__sum16 *csum,
-		      const unsigned char *optr,
-		      const unsigned char *nptr,
-		      int offset)
-{
-	unsigned char s[4];
-
-	if (offset & 1) {
-		s[0] = s[2] = 0;
-		s[1] = ~*optr;
-		s[3] = *nptr;
-	} else {
-		s[1] = s[3] = 0;
-		s[0] = ~*optr;
-		s[2] = *nptr;
-	}
-
-	*csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
-}
-
-/*
  * Mangle IP address.
  * 	- begin points to the start of the snmp messgae
  *      - addr points to the start of the address
@@ -924,11 +900,8 @@ static inline void mangle_address(unsign
 		*addr = map->to;
 
 		/* Update UDP checksum if being used */
-		if (*check) {
-			fast_csum(check,
-				  &map->from, &map->to, addr - begin);
-
-		}
+		if (*check)
+			csum_replace1(check, map->from, map->to);
 
 		if (debug)
 			printk(KERN_DEBUG "bsalg: mapped %pI4 to %pI4\n",
--- a/include/net/checksum.h	2010-09-16 22:31:27.524503074 -0700
+++ b/include/net/checksum.h	2010-09-16 22:32:09.934282263 -0700
@@ -106,6 +106,11 @@ static inline void csum_replace2(__sum16
 	csum_replace4(sum, (__force __be32)from, (__force __be32)to);
 }
 
+static inline void csum_replace1(__sum16 *sum, __u8 from, __u8 to)
+{
+	csum_replace4(sum, (__force __be32)from, (__force __be32)to);
+}
+
 struct sk_buff;
 extern void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
 				     __be32 from, __be32 to, int pseudohdr);
Comment 5 Patrick McHardy 2010-09-17 13:03:08 UTC
Am 17.09.2010 07:39, schrieb Stephen Hemminger:
> nf_nat_snmp: use existing checksum update code
> 
> The fast_csum() in NAT code for processing SNMP trap is buggy 
> (see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
> Replace it by using the existing checksum replacement code;
> it means adding a new csum_replace1() inline wrapper.

Applied, thanks Stephen.
Comment 6 Anonymous Emailer 2010-09-17 15:32:06 UTC
Reply-To: shemminger@vyatta.com

On Fri, 17 Sep 2010 14:08:56 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Am 17.09.2010 07:39, schrieb Stephen Hemminger:
> > nf_nat_snmp: use existing checksum update code
> > 
> > The fast_csum() in NAT code for processing SNMP trap is buggy 
> > (see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
> > Replace it by using the existing checksum replacement code;
> > it means adding a new csum_replace1() inline wrapper.
> 
> Applied, thanks Stephen.

As I said in the patch, this was compile tested only. It would
be good if the original Bug submitter validated that this
fixed the problem.
Comment 7 Patrick McHardy 2010-09-17 15:37:13 UTC
Am 17.09.2010 17:31, schrieb Stephen Hemminger:
> On Fri, 17 Sep 2010 14:08:56 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> Am 17.09.2010 07:39, schrieb Stephen Hemminger:
>>> nf_nat_snmp: use existing checksum update code
>>>
>>> The fast_csum() in NAT code for processing SNMP trap is buggy 
>>> (see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
>>> Replace it by using the existing checksum replacement code;
>>> it means adding a new csum_replace1() inline wrapper.
>>
>> Applied, thanks Stephen.
> 
> As I said in the patch, this was compile tested only. It would
> be good if the original Bug submitter validated that this
> fixed the problem.

Thanks, that slipped past my eyes. I'll make sure to wait for
confirmation from the reporter or try to test this myself before
sending it upstream.
Comment 8 clark wang 2010-09-20 00:37:34 UTC
The way by adding a new csum_replace1() inline wrapper have problem.
And I test it , it can't work. The reason is that it doesn't consider
the value of offset(start addr of snmp message - start addr of agent-addr field) 

I provied a solution to resolve this issue, you can refer to it. And this solution was tested by me, it can work well.

--- old\net\ipv4\netfilter\nf_nat_snmp_basic.c	Wed Sep 15 09:04:30 2010 UTC
+++ new\net\ipv4\netfilter\nf_nat_snmp_basic.c	Wed Sep 15 09:03:43 2010 UTC
@@ -879,10 +879,12 @@
 
       if (offset & 1) {{/*offset is odd numbers, trun a 8-bits into 16-bits by adding the high 8-bits 0x0000*/
 		s[0] = s[2] = 0;
+		s[0] = ~s[0];
 		s[1] = ~*optr;
 		s[3] = *nptr;
 	} else {{/*offset is even numbers, trun a 8-bits into 16-bits by adding the low 8-bits 0x0000*/
 		s[1] = s[3] = 0;
+		s[1] = ~s[1];
 		s[0] = ~*optr;
 		s[2] = *nptr;
 	}

Note You need to log in before you can comment on or make changes to this bug.