* Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue [not found] <bug-17622-10286@https.bugzilla.kernel.org/> @ 2010-09-14 23:32 ` Andrew Morton [not found] ` <6029e4.25bb.12b1d97358d.Coremail.wtweeker@163.com> 1 sibling, 0 replies; 7+ messages in thread From: Andrew Morton @ 2010-09-14 23:32 UTC (permalink / raw) To: wtweeker Cc: bugzilla-daemon, bugme-daemon, netdev, Stephen Hemminger, Patrick McHardy (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. ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <6029e4.25bb.12b1d97358d.Coremail.wtweeker@163.com>]
* Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue [not found] ` <6029e4.25bb.12b1d97358d.Coremail.wtweeker@163.com> @ 2010-09-17 5:39 ` Stephen Hemminger 2010-09-17 12:08 ` Patrick McHardy 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2010-09-17 5:39 UTC (permalink / raw) To: 王韬(计算机科学学院) Cc: akpm, bugzilla-daemon, bugme-daemon, netdev, Patrick McHardy, kaber 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue 2010-09-17 5:39 ` Stephen Hemminger @ 2010-09-17 12:08 ` Patrick McHardy 2010-09-17 15:31 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: Patrick McHardy @ 2010-09-17 12:08 UTC (permalink / raw) To: Stephen Hemminger; +Cc: wtweeker, akpm, bugzilla-daemon, bugme-daemon, netdev 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue 2010-09-17 12:08 ` Patrick McHardy @ 2010-09-17 15:31 ` Stephen Hemminger 2010-09-17 15:36 ` Patrick McHardy [not found] ` <2a014b7c.92ce.12b293c61ba.Coremail.wtweeker@163.com> 0 siblings, 2 replies; 7+ messages in thread From: Stephen Hemminger @ 2010-09-17 15:31 UTC (permalink / raw) To: Patrick McHardy; +Cc: wtweeker, akpm, bugzilla-daemon, bugme-daemon, netdev 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. -- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue 2010-09-17 15:31 ` Stephen Hemminger @ 2010-09-17 15:36 ` Patrick McHardy [not found] ` <2a014b7c.92ce.12b293c61ba.Coremail.wtweeker@163.com> 1 sibling, 0 replies; 7+ messages in thread From: Patrick McHardy @ 2010-09-17 15:36 UTC (permalink / raw) To: Stephen Hemminger; +Cc: wtweeker, akpm, bugzilla-daemon, bugme-daemon, netdev 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <2a014b7c.92ce.12b293c61ba.Coremail.wtweeker@163.com>]
* [PATCH] nf_nat_snmp: fix checksum calculation (v3) [not found] ` <2a014b7c.92ce.12b293c61ba.Coremail.wtweeker@163.com> @ 2010-09-20 16:44 ` Stephen Hemminger 2010-09-20 17:35 ` Patrick McHardy 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2010-09-20 16:44 UTC (permalink / raw) To: 王韬(计算机科学学院) Cc: Patrick McHardy, akpm, bugzilla-daemon, bugme-daemon, netdev Revised version of the original patch in the bug https://bugzilla.kernel.org/show_bug.cgi?id=17622 from clark wang <wtweeker@163.com> I took the opportunity to do some cleanup here. * reorder the assignment to make the byte order clear * get rid of unnecessary ref/deref and just pass the bytes * use sizeof() instead of hard coding size Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> --- --- a/net/ipv4/netfilter/nf_nat_snmp_basic.c 2010-09-20 09:05:11.752067965 -0700 +++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c 2010-09-20 09:38:48.518661840 -0700 @@ -885,24 +885,23 @@ 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) +static void fast_csum(__sum16 *csum, __u8 from, __u8 to, int offset) { - unsigned char s[4]; + __be16 diff[2]; if (offset & 1) { - s[0] = s[2] = 0; - s[1] = ~*optr; - s[3] = *nptr; + s[0] = ~0; + s[1] = ~from; + s[2] = 0; + s[3] = to; } else { - s[1] = s[3] = 0; - s[0] = ~*optr; - s[2] = *nptr; + s[0] = ~from; + s[1] = ~0; + s[2] = to; + s[3] = 0; } - *csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum))); + *csum = csum_fold(csum_partial(s, sizeof(s), ~csum_unfold(*csum))); } /* @@ -924,11 +923,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) + fast_csum(check, map->from, map->to, addr - begin); if (debug) printk(KERN_DEBUG "bsalg: mapped %pI4 to %pI4\n", ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nf_nat_snmp: fix checksum calculation (v3) 2010-09-20 16:44 ` [PATCH] nf_nat_snmp: fix checksum calculation (v3) Stephen Hemminger @ 2010-09-20 17:35 ` Patrick McHardy 0 siblings, 0 replies; 7+ messages in thread From: Patrick McHardy @ 2010-09-20 17:35 UTC (permalink / raw) To: Stephen Hemminger Cc: Íõ躣šŒÆËã»ú¿ÆѧѧԺ£©, akpm, bugzilla-daemon, bugme-daemon, netdev [-- Attachment #1: Type: text/plain, Size: 1069 bytes --] On 20.09.2010 18:44, Stephen Hemminger wrote: > Revised version of the original patch in the bug > https://bugzilla.kernel.org/show_bug.cgi?id=17622 > from clark wang <wtweeker@163.com> > > I took the opportunity to do some cleanup here. > * reorder the assignment to make the byte order clear > * get rid of unnecessary ref/deref and just pass the bytes > * use sizeof() instead of hard coding size > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> Thanks Stephen, but this patch didn't compile: net/ipv4/netfilter/nf_nat_snmp_basic.c: In function 'fast_csum': net/ipv4/netfilter/nf_nat_snmp_basic.c:893: error: 's' undeclared (first use in this function) net/ipv4/netfilter/nf_nat_snmp_basic.c:893: error: (Each undeclared identifier is reported only once net/ipv4/netfilter/nf_nat_snmp_basic.c:893: error: for each function it appears in.) net/ipv4/netfilter/nf_nat_snmp_basic.c:890: warning: unused variable 'diff' Since I prefer to keep this fix to the minimal size at this point, I've committed this patch based on Clark's and your patches: [-- Attachment #2: x --] [-- Type: text/plain, Size: 1031 bytes --] commit 8d70d82cdcc6da0a77440c6d860957a1f50b8089 Author: Patrick McHardy <kaber@trash.net> Date: Mon Sep 20 19:29:40 2010 +0200 netfilter: nf_nat_snmp: fix checksum calculation (v4) Fix checksum calculation in nf_nat_snmp_basic. Based on patches by Clark Wang <wtweeker@163.com> and Stephen Hemminger <shemminger@vyatta.com>. https://bugzilla.kernel.org/show_bug.cgi?id=17622 Signed-off-by: Patrick McHardy <kaber@trash.net> diff --git a/net/ipv4/netfilter/nf_nat_snmp_basic.c b/net/ipv4/netfilter/nf_nat_snmp_basic.c index 1679e2c..ee5f419 100644 --- a/net/ipv4/netfilter/nf_nat_snmp_basic.c +++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c @@ -893,13 +893,15 @@ static void fast_csum(__sum16 *csum, unsigned char s[4]; if (offset & 1) { - s[0] = s[2] = 0; + s[0] = ~0; s[1] = ~*optr; + s[2] = 0; s[3] = *nptr; } else { - s[1] = s[3] = 0; s[0] = ~*optr; + s[1] = ~0; s[2] = *nptr; + s[3] = 0; } *csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum))); ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-09-20 17:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <bug-17622-10286@https.bugzilla.kernel.org/> 2010-09-14 23:32 ` [Bugme-new] [Bug 17622] New: snmp trap ALG issue Andrew Morton [not found] ` <6029e4.25bb.12b1d97358d.Coremail.wtweeker@163.com> 2010-09-17 5:39 ` Stephen Hemminger 2010-09-17 12:08 ` Patrick McHardy 2010-09-17 15:31 ` Stephen Hemminger 2010-09-17 15:36 ` Patrick McHardy [not found] ` <2a014b7c.92ce.12b293c61ba.Coremail.wtweeker@163.com> 2010-09-20 16:44 ` [PATCH] nf_nat_snmp: fix checksum calculation (v3) Stephen Hemminger 2010-09-20 17:35 ` Patrick McHardy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.