Hi!
I have found a few days ago strange messages in /var/log/messages
More than 10 times there was do_IRQ: stack overflow: (nimber).... followed
with code. If need I can send all this data. I have run
ksymoops with only first 3 cases. Here is the first, the second and
the third are in attachment.
After that oopses my system continued to work.
It's not really an oops, just a warning that stack space got quiet tight.
The problem seems to be that the br netfilter code is nesting far too
deeply and recursing several times. Looks like a design bug to me,
it shouldn't do that.
uname uname -a
Linux linux 2.4.28 #2 ÷ÔÒ îÏÑ 30 15:43:35 MSK 2004 i686 unknown
gcc -v
Reading specs from /usr/lib/gcc-lib/i386-redhat-linux/2.96/specs
gcc version 2.96 20000731 (Red Hat Linux 7.3 2.96-113)
I have applies ebtables_brnf patch (http://bridge.sf.net) and a
Don't do that then or contact the author to fix it. Unfortunately
the code is also in 2.6 mainline.
-Andi
From: Bart De Schuymer <hidden> Date: 2004-12-18 11:08:43
Op za, 18-12-2004 te 08:50 +0100, schreef Andi Kleen:
Crazy AMD K7 [off-list ref] writes:
quoted
Hi!
I have found a few days ago strange messages in /var/log/messages
More than 10 times there was do_IRQ: stack overflow: (nimber).... followed
with code. If need I can send all this data. I have run
ksymoops with only first 3 cases. Here is the first, the second and
the third are in attachment.
After that oopses my system continued to work.
It's not really an oops, just a warning that stack space got quiet tight.
The problem seems to be that the br netfilter code is nesting far too
deeply and recursing several times. Looks like a design bug to me,
it shouldn't do that.
quoted
uname uname -a
Linux linux 2.4.28 #2 ÷ÔÒ îÏÑ 30 15:43:35 MSK 2004 i686 unknown
gcc -v
Reading specs from /usr/lib/gcc-lib/i386-redhat-linux/2.96/specs
gcc version 2.96 20000731 (Red Hat Linux 7.3 2.96-113)
I have applies ebtables_brnf patch (http://bridge.sf.net) and a
Don't do that then or contact the author to fix it. Unfortunately
the code is also in 2.6 mainline.
Hi.
The bridge-nf code does not use recursive function calls and there is no
long consecutive function calling. Furthermore, there is no function in
the bridge-nf code that uses a large part of the stack.
Andi, if you make such statements then please point out the code part
you have (of course) read after which you decided to make the statement.
The bridge-nf code is used by quite a few people and by commercial
companies and I have never had a report like this. AMD has been having
all sorts of strange problems for weeks now, they're all somehow related
to bridge-nf, but I doubt he is using the bridge-nf patch on a clean 2.4
kernel.
AMD, is there any chance you can use the latest 2.6 kernel, without
extra patches ?
Bart
The bridge-nf code does not use recursive function calls and there is no
long consecutive function calling. Furthermore, there is no function in
the bridge-nf code that uses a large part of the stack.
Just take a look at the backtrace in the original post. It clearly
shows a problem. And it points strongly towards br-netfilter.
From: Bart De Schuymer <hidden> Date: 2004-12-18 11:48:39
Op za, 18-12-2004 te 12:14 +0100, schreef Andi Kleen:
quoted
The bridge-nf code does not use recursive function calls and there is no
long consecutive function calling. Furthermore, there is no function in
the bridge-nf code that uses a large part of the stack.
Just take a look at the backtrace in the original post. It clearly
shows a problem. And it points strongly towards br-netfilter.
I don't doubt you are a much better reader of such backtraces than me.
However, let's count the number of times a function from
net/bridge/br_netfilter.c is in the backtrace:
1. br_nf*: 6 times
2. *sabotage*: 3 times
Seriously, out of 222 lines, only 9 from bridge-nf.
The function ip_queue_xmit, OTOH, is 8 times in the trace.
Anyway, as I already suspected weeks ago, AMD must be seeing some
incompatibility between ip_queue (he's using snort) and the bridge-nf
patch.
He is using the patch (I gave it to him) below on top of the bridge-nf
patch. Before using that patch he got a kernel panic occasionally.
However he seems not to get a message in his syslog.
Bart
@@ -884,6 +888,10 @@ static unsigned int ip_sabotage_out(unsi } #endif+if (!out) {+ printk("TROUBLE IN IP_SABOTAGE_OUT: out == NULL\n");+ goto in_trouble;+} if ((out->hard_start_xmit == br_dev_xmit && okfn != br_nf_forward_finish && okfn != br_nf_local_out_finish &&
@@ -920,6 +928,9 @@ static unsigned int ip_sabotage_out(unsi } return NF_ACCEPT;+in_trouble:+ dump_stack();+ return NF_DROP; } /* For br_nf_local_out we need (prio = NF_BR_PRI_FIRST), to insure that
On Sat, Dec 18, 2004 at 12:51:30PM +0100, Bart De Schuymer wrote:
quoted
Just take a look at the backtrace in the original post. It clearly
shows a problem. And it points strongly towards br-netfilter.
I don't doubt you are a much better reader of such backtraces than me.
However, let's count the number of times a function from
net/bridge/br_netfilter.c is in the backtrace:
1. br_nf*: 6 times
2. *sabotage*: 3 times
Seriously, out of 222 lines, only 9 from bridge-nf.
The function ip_queue_xmit, OTOH, is 8 times in the trace.
Yep, but ip_queue_xmit doesn't call itself recursively. Someone
must be doing it. And that's likely the bridge code.
BTW not all of these entries are probably true, there can
be a lot of false positives.
Anyway, as I already suspected weeks ago, AMD must be seeing some
incompatibility between ip_queue (he's using snort) and the bridge-nf
patch.
He is using the patch (I gave it to him) below on top of the bridge-nf
patch. Before using that patch he got a kernel panic occasionally.
However he seems not to get a message in his syslog.
Ok, since this report seems to be for a totally non standard
severly hacked up kernel I suppose nothing from it can be concluded for
the mainline kernel. Thanks for clearing this up.
Note to the original poster: when you report a bug with a patched
kernel always mention it.
-Andi
Note to the original poster: when you report a bug with a patched
kernel always mention it.
I have mentioned earlier and Bart knows it.
I use 2.4.28
+ ebtables-brnf-8_vs_2.4.28.diff
+ U32 patch from patch-o-matic-ng-20040621.tar.bz2
+ patch for br_netfilter.c made by Bart to find out why kernel panic happens(it was a few
letters ago)
All patches has applies cleanly.
U32 doesn't affect on br_netfilter.c
[root@linux kernel]# md5sum linux-2.4.28.tar.bz2
ac7735000d185bc7778c08288760a8a3 linux-2.4.28.tar.bz2
(taken from http://www.ru.kernel.org/pub/linux/kernel/v2.4/linux-2.4.28.tar.bz2)
[root@linux bridge]# md5sum ebtables-brnf-8_vs_2.4.28.diff.gz
30542b1a7a502593afb4d37055ec5e35 ebtables-brnf-8_vs_2.4.28.diff.gz
[root@linux iptables]# md5sum patch-o-matic-ng-20040621.tar.bz2
4fd3c744bf55f119fef6c7c3c4acc4b6 patch-o-matic-ng-20040621.tar.bz2
If the problem will continue appear and will not be solved an any
way, of course, I will use 2.6 kernel, now I am not ready to use it.
Pasha
From: Bart De Schuymer <hidden> Date: 2004-12-18 16:43:26
Op za, 18-12-2004 te 19:07 +0300, schreef Crazy AMD K7:
quoted
Note to the original poster: when you report a bug with a patched
kernel always mention it.
I have mentioned earlier and Bart knows it.
I use 2.4.28
+ ebtables-brnf-8_vs_2.4.28.diff
+ U32 patch from patch-o-matic-ng-20040621.tar.bz2
+ patch for br_netfilter.c made by Bart to find out why kernel panic happens(it was a few
letters ago)
All patches has applies cleanly.
U32 doesn't affect on br_netfilter.c
Sorry, I don't know the ip_queue mechanism and I don't know what could
possibly go wrong.
All we know is that you no longer have kernel panics with the simple
patch I gave you (which just drops packets when a kernel panic would
happen otherwise, and tells about this with a printk). However, you
state there are no entries in your syslog that tell about this dropping.
Is your syslog working right? Do you have a console open on which kernel
messages get printed?
I still secretly suspect the snort code of inserting packets back into
the kernel that don't have an output device (I don't know if that's
possible, though).
cheers,
Bart
From: David Woodhouse <dwmw2@infradead.org> Date: 2005-01-07 17:12:37
On Sat, 2004-12-18 at 08:50 +0100, Andi Kleen wrote:
It's not really an oops, just a warning that stack space got quiet
tight.
The problem seems to be that the br netfilter code is nesting far too
deeply and recursing several times. Looks like a design bug to me,
it shouldn't do that.
I don't think it's recursing -- I think the stack trace is just a bit
noisy. The problem is that the bridge code, especially with br_netfilter
in the equation, is implicated in code paths which are just _too_ deep.
This happens when you're bridging packets received in an interrupt while
you were deep in journalling code, and it's also been seen with a call
trace something like nfs->sunrpc->ip->bridge->br_netfilter.
One option might be to make br_dev_xmit() just queue the packet rather
than trying to deliver it to all the slave devices immediately. Then the
actual retransmission can be handled from a context where we're _not_
short of stack; perhaps from a dedicated kernel thread.
Unfortunately that approach would introduce a lot of latency on all
packets we pass. Another option would be to have all architectures
provide a stack_available() function and for br_dev_xmit() to queue the
packet only if we're short of stack, while still sending most packets
immediately.
Proof of concept below; obviously the stack_available() is an evil hack
and would need to be done more sanely. Comments?
===== net/bridge/br_device.c 1.17 vs edited =====
From: Stephen Hemminger <hidden> Date: 2005-01-07 18:00:17
On Fri, 07 Jan 2005 17:05:59 +0000
David Woodhouse [off-list ref] wrote:
On Sat, 2004-12-18 at 08:50 +0100, Andi Kleen wrote:
quoted
It's not really an oops, just a warning that stack space got quiet
tight.
The problem seems to be that the br netfilter code is nesting far too
deeply and recursing several times. Looks like a design bug to me,
it shouldn't do that.
I don't think it's recursing -- I think the stack trace is just a bit
noisy. The problem is that the bridge code, especially with br_netfilter
in the equation, is implicated in code paths which are just _too_ deep.
This happens when you're bridging packets received in an interrupt while
you were deep in journalling code, and it's also been seen with a call
trace something like nfs->sunrpc->ip->bridge->br_netfilter.
Sounds like an argument for interrupt stacks.
One option might be to make br_dev_xmit() just queue the packet rather
than trying to deliver it to all the slave devices immediately. Then the
actual retransmission can be handled from a context where we're _not_
short of stack; perhaps from a dedicated kernel thread.
Probably the solution would be to handle it in the filter code
that way if we are not filtering, we can use the interrupt path,
but if filtering just defer to a safer context (like soft irq).
Unfortunately that approach would introduce a lot of latency on all
packets we pass. Another option would be to have all architectures
provide a stack_available() function and for br_dev_xmit() to queue the
packet only if we're short of stack, while still sending most packets
immediately.
NO, that looks like a testablity and portablity nightmare.
From: David Woodhouse <dwmw2@infradead.org> Date: 2005-01-07 18:06:52
On Fri, 2005-01-07 at 10:00 -0800, Stephen Hemminger wrote:
quoted
I don't think it's recursing -- I think the stack trace is just a bit
noisy. The problem is that the bridge code, especially with br_netfilter
in the equation, is implicated in code paths which are just _too_ deep.
This happens when you're bridging packets received in an interrupt while
you were deep in journalling code, and it's also been seen with a call
trace something like nfs->sunrpc->ip->bridge->br_netfilter.
Sounds like an argument for interrupt stacks.
The NFS case didn't involve hardware interrupts. Except for the one
which actually detected that the stack had overflowed.
Probably the solution would be to handle it in the filter code
that way if we are not filtering, we can use the interrupt path,
but if filtering just defer to a safer context (like soft irq).
That's also a possibility.
quoted
Unfortunately that approach would introduce a lot of latency on all
packets we pass. Another option would be to have all architectures
provide a stack_available() function and for br_dev_xmit() to queue the
packet only if we're short of stack, while still sending most packets
immediately.
NO, that looks like a testablity and portablity nightmare.
From: Bart De Schuymer <hidden> Date: 2005-01-07 21:27:21
Op vr, 07-01-2005 te 10:00 -0800, schreef Stephen Hemminger:
On Fri, 07 Jan 2005 17:05:59 +0000
David Woodhouse [off-list ref] wrote:
quoted
I don't think it's recursing -- I think the stack trace is just a bit
noisy. The problem is that the bridge code, especially with br_netfilter
in the equation, is implicated in code paths which are just _too_ deep.
This happens when you're bridging packets received in an interrupt while
you were deep in journalling code, and it's also been seen with a call
trace something like nfs->sunrpc->ip->bridge->br_netfilter.
Sounds like an argument for interrupt stacks.
quoted
One option might be to make br_dev_xmit() just queue the packet rather
than trying to deliver it to all the slave devices immediately. Then the
actual retransmission can be handled from a context where we're _not_
short of stack; perhaps from a dedicated kernel thread.
Probably the solution would be to handle it in the filter code
that way if we are not filtering, we can use the interrupt path,
but if filtering just defer to a safer context (like soft irq).
How about something like the patch below (untested but compiles)?
The current netfilter scheme adds one function call to the call chain
for each NF_HOOK and NF_HOOK_THRESH. This can be prevented by executing
the okfn in the calling function instead of in nf_hook_slow().
I didn't check if there's any code that actually uses the return value
from NF_HOOK. If so, this patch won't work well in its current form as -
EPERM is now also returned for NF_QUEUE and NF_STOLEN.
Another 2 calls of okfn can be postponed in br_netfilter.c by adding
NF_STOP, which would work like NF_STOLEN except that okfn is still
called. But I'd first like to get the IPv4/IPv6 fix for br_netfilter.c
accepted (see another thread on netdev).
cheers,
Bart
From: "David S. Miller" <davem@davemloft.net> Date: 2005-01-18 21:57:35
On Fri, 07 Jan 2005 22:27:21 +0100
Bart De Schuymer [off-list ref] wrote:
How about something like the patch below (untested but compiles)?
The current netfilter scheme adds one function call to the call chain
for each NF_HOOK and NF_HOOK_THRESH. This can be prevented by executing
the okfn in the calling function instead of in nf_hook_slow().
I didn't check if there's any code that actually uses the return value
from NF_HOOK. If so, this patch won't work well in its current form as -
EPERM is now also returned for NF_QUEUE and NF_STOLEN.
Another 2 calls of okfn can be postponed in br_netfilter.c by adding
NF_STOP, which would work like NF_STOLEN except that okfn is still
called. But I'd first like to get the IPv4/IPv6 fix for br_netfilter.c
accepted (see another thread on netdev).
I believe I put in your ipv4/ipv6 br_netfilter fix already.
This NF_HOOK() change looks interesting. Could we also do something like
running the deeper ->hard_start_xmit() via a triggered tasklet or something
similar?
From: Bart De Schuymer <hidden> Date: 2005-01-22 22:30:59
Op di, 18-01-2005 te 13:57 -0800, schreef David S. Miller:
On Fri, 07 Jan 2005 22:27:21 +0100
Bart De Schuymer [off-list ref] wrote:
quoted
How about something like the patch below (untested but compiles)?
The current netfilter scheme adds one function call to the call chain
for each NF_HOOK and NF_HOOK_THRESH. This can be prevented by executing
the okfn in the calling function instead of in nf_hook_slow().
I didn't check if there's any code that actually uses the return value
from NF_HOOK. If so, this patch won't work well in its current form as -
EPERM is now also returned for NF_QUEUE and NF_STOLEN.
Another 2 calls of okfn can be postponed in br_netfilter.c by adding
NF_STOP, which would work like NF_STOLEN except that okfn is still
called.
This NF_HOOK() change looks interesting. Could we also do something like
running the deeper ->hard_start_xmit() via a triggered tasklet or something
similar?
Hi,
The patch below reduces the call chain length for netfilter hooks by
executing the okfn() inside the caller function instead of inside
nf_hook_slow(). It also reduces the size of netfilter.o from 129590 to
129550 on my system.
The return value for NF_HOOK stays the same as before: 0 except when the
verdict was NF_DROP (in that case the return value becomes -EPERM).
The target NF_STOP is added to postpone two executions of okfn() in
br_netfilter.c, it works like NF_STOLEN except that the okfn() will
still be called.
The downside is that all .o files using NF_HOOK will enlarge.
Comments are welcome.
cheers,
Bart
Signed-off-by: Bart De Schuymer <redacted>
@@ -349,6 +349,8 @@ static unsigned int nf_iterate(struct liint(*okfn)(structsk_buff*),inthook_thresh){+unsignedintverdict;+/**Thecallermustnotblockbetweencallstothis*functionbecauseofriskofcontinuingfromdeletedelement.
@@ -361,28 +363,18 @@ static unsigned int nf_iterate(struct li/* Optimization: we don't need to hold modulereferencehere,sincefunctioncan'tsleep.--RR*/-switch(elem->hook(hook,skb,indev,outdev,okfn)){-caseNF_QUEUE:-returnNF_QUEUE;--caseNF_STOLEN:-returnNF_STOLEN;--caseNF_DROP:-returnNF_DROP;--caseNF_REPEAT:-*i=(*i)->prev;-break;-+verdict=elem->hook(hook,skb,indev,outdev,okfn);+if(verdict!=NF_ACCEPT){#ifdef CONFIG_NETFILTER_DEBUG-caseNF_ACCEPT:-break;--default:-NFDEBUG("Evil return from %p(%u).\n",-elem->hook,hook);+if(verdict>NF_MAX_VERDICT){+NFDEBUG("Evil return from %p(%u).\n",+elem->hook,hook);+continue;+}#endif+if(verdict!=NF_REPEAT)+returnverdict;+*i=(*i)->prev;}}returnNF_ACCEPT;
@@ -494,50 +486,47 @@ static int nf_queue(struct sk_buff *skb,return1;}-intnf_hook_slow(intpf,unsignedinthook,structsk_buff*skb,+/* Returns 0 if okfn() needs to be executed by the caller, -EPERM otherwise.+*Assumes*ret==0whencalled.Onreturn,*ret!=0whenverdict==NF_DROP*/+intnf_hook_slow(intpf,unsignedinthook,structsk_buff**pskb,structnet_device*indev,structnet_device*outdev,int(*okfn)(structsk_buff*),-inthook_thresh)+inthook_thresh,int*ret){structlist_head*elem;unsignedintverdict;-intret=0;+intret2=0;/* We may already have this, but read-locks nest anyway */rcu_read_lock();#ifdef CONFIG_NETFILTER_DEBUG-if(skb->nf_debug&(1<<hook)){+if((*pskb)->nf_debug&(1<<hook)){printk("nf_hook: hook %i already set.\n",hook);-nf_dump_skb(pf,skb);+nf_dump_skb(pf,*pskb);}skb->nf_debug|=(1<<hook);#endifelem=&nf_hooks[pf][hook];next_hook:-verdict=nf_iterate(&nf_hooks[pf][hook],&skb,hook,indev,+verdict=nf_iterate(&nf_hooks[pf][hook],pskb,hook,indev,outdev,&elem,okfn,hook_thresh);-if(verdict==NF_QUEUE){+if(verdict==NF_ACCEPT||verdict==NF_STOP)+gotounlock;+elseif(verdict==NF_DROP){+kfree_skb(*pskb);+*ret=-EPERM;+}elseif(verdict==NF_QUEUE){NFDEBUG("nf_hook: Verdict = QUEUE.\n");-if(!nf_queue(skb,elem,pf,hook,indev,outdev,okfn))+if(!nf_queue(*pskb,elem,pf,hook,indev,outdev,okfn))gotonext_hook;}--switch(verdict){-caseNF_ACCEPT:-ret=okfn(skb);-break;--caseNF_DROP:-kfree_skb(skb);-ret=-EPERM;-break;-}-+ret2=-EPERM;+unlock:rcu_read_unlock();-returnret;+returnret2;}voidnf_reinject(structsk_buff*skb,structnf_info*info,---linux-2.6.1-rc1/net/bridge/br_netfilter.c.old2005-01-2021:43:08.000000000+0100+++linux-2.6.1-rc1/net/bridge/br_netfilter.c2005-01-2221:20:06.000000000+0100
@@ -829,8 +829,7 @@ static unsigned int ip_sabotage_in(unsig{if((*pskb)->nf_bridge&&!((*pskb)->nf_bridge->mask&BRNF_NF_BRIDGE_PREROUTING)){-okfn(*pskb);-returnNF_STOLEN;+returnNF_STOP;}returnNF_ACCEPT;
@@ -888,8 +887,7 @@ static unsigned int ip_sabotage_out(unsiif(out->priv_flags&IFF_802_1Q_VLAN)nf_bridge->netoutdev=(structnet_device*)out;#endif-okfn(skb);-returnNF_STOLEN;+returnNF_STOP;}returnNF_ACCEPT;
From: Martin Josefsson <hidden> Date: 2005-01-22 23:22:33
On Sat, 2005-01-22 at 23:30 +0100, Bart De Schuymer wrote:
Hi Bart
quoted hunk
@@ -138,23 +139,34 @@ void nf_log_packet(int pf, /* This is gross, but inline doesn't cut it for avoiding the function call in fast path: gcc doesn't inline (needs value tracking?). --RR */ #ifdef CONFIG_NETFILTER_DEBUG-#define NF_HOOK(pf, hook, skb, indev, outdev, okfn) \- nf_hook_slow((pf), (hook), (skb), (indev), (outdev), (okfn), INT_MIN)-#define NF_HOOK_THRESH nf_hook_slow+#define NF_HOOK(pf, hook, skb, indev, outdev, okfn) \+({int __ret = 0; \+if (!nf_hook_slow(pf, hook, &(skb), indev, outdev, okfn, INT_MIN, &__ret); \+ __ret = (okfn)(skb); \+__ret;})+#define NF_HOOK_THRESH(pf, hook, skb, indev, outdev, okfn, thresh) \+({int __ret = 0; \+if (!nf_hook_slow(pf, hook, &(skb), indev, outdev, okfn, thresh, &__ret); \+ __ret = (okfn)(skb); \+__ret;}) #else
I guess you never testcompiled with CONFIG_NETFILTER_DEBUG set :)
The if-statements above needs to have ; replaced with )
+ if (verdict > NF_MAX_VERDICT) {
+ NFDEBUG("Evil return from %p(%u).\n",
+ elem->hook, hook);
+ continue;
+ }
Maybe add unlikely() around the test?
Otherwise the changes look sane.
The reoganisation of things in nf_hook_slow() shouldn't cause any
performance changes, I tried to benchmark various variations of that
code some time ago but the result of the changes were more or less in
the noise.
--
/Martin
From: Bart De Schuymer <hidden> Date: 2005-01-23 12:40:19
Op zo, 23-01-2005 te 00:22 +0100, schreef Martin Josefsson:
On Sat, 2005-01-22 at 23:30 +0100, Bart De Schuymer wrote:
Hi Bart
Hi Martin,
I guess you never testcompiled with CONFIG_NETFILTER_DEBUG set :)
Oops, no. I was imagining I could code that without error...
There was another compile error with CONFIG_NETFILTER_DEBUG in the patch
(skb that should have been (*pskb)) :(
quoted
+ if (verdict > NF_MAX_VERDICT) {
+ NFDEBUG("Evil return from %p(%u).\n",
+ elem->hook, hook);
+ continue;
+ }
Maybe add unlikely() around the test?
OK. I added another one around another debugging if-statement.
cheers,
Bart
@@ -349,6 +349,8 @@ static unsigned int nf_iterate(struct liint(*okfn)(structsk_buff*),inthook_thresh){+unsignedintverdict;+/**Thecallermustnotblockbetweencallstothis*functionbecauseofriskofcontinuingfromdeletedelement.
@@ -361,28 +363,18 @@ static unsigned int nf_iterate(struct li/* Optimization: we don't need to hold modulereferencehere,sincefunctioncan'tsleep.--RR*/-switch(elem->hook(hook,skb,indev,outdev,okfn)){-caseNF_QUEUE:-returnNF_QUEUE;--caseNF_STOLEN:-returnNF_STOLEN;--caseNF_DROP:-returnNF_DROP;--caseNF_REPEAT:-*i=(*i)->prev;-break;-+verdict=elem->hook(hook,skb,indev,outdev,okfn);+if(verdict!=NF_ACCEPT){#ifdef CONFIG_NETFILTER_DEBUG-caseNF_ACCEPT:-break;--default:-NFDEBUG("Evil return from %p(%u).\n",-elem->hook,hook);+if(unlikely(verdict>NF_MAX_VERDICT)){+NFDEBUG("Evil return from %p(%u).\n",+elem->hook,hook);+continue;+}#endif+if(verdict!=NF_REPEAT)+returnverdict;+*i=(*i)->prev;}}returnNF_ACCEPT;
@@ -494,50 +486,47 @@ static int nf_queue(struct sk_buff *skb,return1;}-intnf_hook_slow(intpf,unsignedinthook,structsk_buff*skb,+/* Returns 0 if okfn() needs to be executed by the caller, -EPERM otherwise.+*Assumes*ret==0whencalled.Onreturn,*ret!=0whenverdict==NF_DROP*/+intnf_hook_slow(intpf,unsignedinthook,structsk_buff**pskb,structnet_device*indev,structnet_device*outdev,int(*okfn)(structsk_buff*),-inthook_thresh)+inthook_thresh,int*ret){structlist_head*elem;unsignedintverdict;-intret=0;+intret2=0;/* We may already have this, but read-locks nest anyway */rcu_read_lock();#ifdef CONFIG_NETFILTER_DEBUG-if(skb->nf_debug&(1<<hook)){+if(unlikely((*pskb)->nf_debug&(1<<hook))){printk("nf_hook: hook %i already set.\n",hook);-nf_dump_skb(pf,skb);+nf_dump_skb(pf,*pskb);}-skb->nf_debug|=(1<<hook);+(*pskb)->nf_debug|=(1<<hook);#endifelem=&nf_hooks[pf][hook];next_hook:-verdict=nf_iterate(&nf_hooks[pf][hook],&skb,hook,indev,+verdict=nf_iterate(&nf_hooks[pf][hook],pskb,hook,indev,outdev,&elem,okfn,hook_thresh);-if(verdict==NF_QUEUE){+if(verdict==NF_ACCEPT||verdict==NF_STOP)+gotounlock;+elseif(verdict==NF_DROP){+kfree_skb(*pskb);+*ret=-EPERM;+}elseif(verdict==NF_QUEUE){NFDEBUG("nf_hook: Verdict = QUEUE.\n");-if(!nf_queue(skb,elem,pf,hook,indev,outdev,okfn))+if(!nf_queue(*pskb,elem,pf,hook,indev,outdev,okfn))gotonext_hook;}--switch(verdict){-caseNF_ACCEPT:-ret=okfn(skb);-break;--caseNF_DROP:-kfree_skb(skb);-ret=-EPERM;-break;-}-+ret2=-EPERM;+unlock:rcu_read_unlock();-returnret;+returnret2;}voidnf_reinject(structsk_buff*skb,structnf_info*info,---linux-2.6.11-rc1/net/bridge/br_netfilter.c.old2005-01-2313:31:39.080899144+0100+++linux-2.6.11-rc1/net/bridge/br_netfilter.c2005-01-2313:32:02.861283976+0100
@@ -829,8 +829,7 @@ static unsigned int ip_sabotage_in(unsig{if((*pskb)->nf_bridge&&!((*pskb)->nf_bridge->mask&BRNF_NF_BRIDGE_PREROUTING)){-okfn(*pskb);-returnNF_STOLEN;+returnNF_STOP;}returnNF_ACCEPT;
@@ -888,8 +887,7 @@ static unsigned int ip_sabotage_out(unsiif(out->priv_flags&IFF_802_1Q_VLAN)nf_bridge->netoutdev=(structnet_device*)out;#endif-okfn(skb);-returnNF_STOLEN;+returnNF_STOP;}returnNF_ACCEPT;
From: Martin Josefsson <hidden> Date: 2005-01-23 16:08:29
On Sun, 2005-01-23 at 13:40 +0100, Bart De Schuymer wrote:
Hi Martin,
Hi Bart
quoted
I guess you never testcompiled with CONFIG_NETFILTER_DEBUG set :)
Oops, no. I was imagining I could code that without error...
There was another compile error with CONFIG_NETFILTER_DEBUG in the patch
(skb that should have been (*pskb)) :(
:)
quoted
quoted
+ if (verdict > NF_MAX_VERDICT) {
+ NFDEBUG("Evil return from %p(%u).\n",
+ elem->hook, hook);
+ continue;
+ }
Maybe add unlikely() around the test?
OK. I added another one around another debugging if-statement.
I'm now running a kernel with this patch and everything seems to still
be working.
So unless someone else has something to comment I think this should be
applied.
The decrease in call-depth is important.
--
/Martin
From: "David S. Miller" <davem@davemloft.net> Date: 2005-01-26 06:05:58
On Sun, 23 Jan 2005 17:08:29 +0100
Martin Josefsson [off-list ref] wrote:
I'm now running a kernel with this patch and everything seems to still
be working.
So unless someone else has something to comment I think this should be
applied.
The decrease in call-depth is important.
I would like to see at least one ACK from the netfilter
folks. Bart or Rusty, could you forward to patch to
netfilter-devel for review?
I have some other ideas about how bridging might be able
to save some call chain depth... but I need to think about
it some more before proposing or even trying to implement.
(basically something akin to how we do route level packet
output, via dst_output(), but instead we're doing this
at ->hard_start_xmit() time)
From: Bart De Schuymer <hidden> Date: 2005-01-26 09:08:29
Op di, 25-01-2005 te 22:05 -0800, schreef David S. Miller:
On Sun, 23 Jan 2005 17:08:29 +0100
Martin Josefsson [off-list ref] wrote:
quoted
I'm now running a kernel with this patch and everything seems to still
be working.
So unless someone else has something to comment I think this should be
applied.
The decrease in call-depth is important.
I would like to see at least one ACK from the netfilter
folks. Bart or Rusty, could you forward to patch to
netfilter-devel for review?
AFAIK Martin is in the netfilter core team. Anyway, I just included
netfilter-devel.
Does anyone have objections to this patch, which reduces the netfilter
call chain length?
I have some other ideas about how bridging might be able
to save some call chain depth... but I need to think about
it some more before proposing or even trying to implement.
(basically something akin to how we do route level packet
output, via dst_output(), but instead we're doing this
at ->hard_start_xmit() time)
@@ -349,6 +349,8 @@ static unsigned int nf_iterate(struct liint(*okfn)(structsk_buff*),inthook_thresh){+unsignedintverdict;+/**Thecallermustnotblockbetweencallstothis*functionbecauseofriskofcontinuingfromdeletedelement.
@@ -361,28 +363,18 @@ static unsigned int nf_iterate(struct li/* Optimization: we don't need to hold modulereferencehere,sincefunctioncan'tsleep.--RR*/-switch(elem->hook(hook,skb,indev,outdev,okfn)){-caseNF_QUEUE:-returnNF_QUEUE;--caseNF_STOLEN:-returnNF_STOLEN;--caseNF_DROP:-returnNF_DROP;--caseNF_REPEAT:-*i=(*i)->prev;-break;-+verdict=elem->hook(hook,skb,indev,outdev,okfn);+if(verdict!=NF_ACCEPT){#ifdef CONFIG_NETFILTER_DEBUG-caseNF_ACCEPT:-break;--default:-NFDEBUG("Evil return from %p(%u).\n",-elem->hook,hook);+if(unlikely(verdict>NF_MAX_VERDICT)){+NFDEBUG("Evil return from %p(%u).\n",+elem->hook,hook);+continue;+}#endif+if(verdict!=NF_REPEAT)+returnverdict;+*i=(*i)->prev;}}returnNF_ACCEPT;
@@ -494,50 +486,47 @@ static int nf_queue(struct sk_buff *skb,return1;}-intnf_hook_slow(intpf,unsignedinthook,structsk_buff*skb,+/* Returns 0 if okfn() needs to be executed by the caller, -EPERM otherwise.+*Assumes*ret==0whencalled.Onreturn,*ret!=0whenverdict==NF_DROP*/+intnf_hook_slow(intpf,unsignedinthook,structsk_buff**pskb,structnet_device*indev,structnet_device*outdev,int(*okfn)(structsk_buff*),-inthook_thresh)+inthook_thresh,int*ret){structlist_head*elem;unsignedintverdict;-intret=0;+intret2=0;/* We may already have this, but read-locks nest anyway */rcu_read_lock();#ifdef CONFIG_NETFILTER_DEBUG-if(skb->nf_debug&(1<<hook)){+if(unlikely((*pskb)->nf_debug&(1<<hook))){printk("nf_hook: hook %i already set.\n",hook);-nf_dump_skb(pf,skb);+nf_dump_skb(pf,*pskb);}-skb->nf_debug|=(1<<hook);+(*pskb)->nf_debug|=(1<<hook);#endifelem=&nf_hooks[pf][hook];next_hook:-verdict=nf_iterate(&nf_hooks[pf][hook],&skb,hook,indev,+verdict=nf_iterate(&nf_hooks[pf][hook],pskb,hook,indev,outdev,&elem,okfn,hook_thresh);-if(verdict==NF_QUEUE){+if(verdict==NF_ACCEPT||verdict==NF_STOP)+gotounlock;+elseif(verdict==NF_DROP){+kfree_skb(*pskb);+*ret=-EPERM;+}elseif(verdict==NF_QUEUE){NFDEBUG("nf_hook: Verdict = QUEUE.\n");-if(!nf_queue(skb,elem,pf,hook,indev,outdev,okfn))+if(!nf_queue(*pskb,elem,pf,hook,indev,outdev,okfn))gotonext_hook;}--switch(verdict){-caseNF_ACCEPT:-ret=okfn(skb);-break;--caseNF_DROP:-kfree_skb(skb);-ret=-EPERM;-break;-}-+ret2=-EPERM;+unlock:rcu_read_unlock();-returnret;+returnret2;}voidnf_reinject(structsk_buff*skb,structnf_info*info,---linux-2.6.11-rc1/net/bridge/br_netfilter.c.old2005-01-2313:31:39.080899144+0100+++linux-2.6.11-rc1/net/bridge/br_netfilter.c2005-01-2313:32:02.861283976+0100
@@ -829,8 +829,7 @@ static unsigned int ip_sabotage_in(unsig{if((*pskb)->nf_bridge&&!((*pskb)->nf_bridge->mask&BRNF_NF_BRIDGE_PREROUTING)){-okfn(*pskb);-returnNF_STOLEN;+returnNF_STOP;}returnNF_ACCEPT;
@@ -888,8 +887,7 @@ static unsigned int ip_sabotage_out(unsiif(out->priv_flags&IFF_802_1Q_VLAN)nf_bridge->netoutdev=(structnet_device*)out;#endif-okfn(skb);-returnNF_STOLEN;+returnNF_STOP;}returnNF_ACCEPT;
From: "David S. Miller" <davem@davemloft.net> Date: 2005-01-27 07:18:01
On Thu, 27 Jan 2005 00:49:01 +0100
Patrick McHardy [off-list ref] wrote:
Bart De Schuymer wrote:
quoted
Does anyone have objections to this patch, which reduces the netfilter
call chain length?
Looks fine to me.
Signed-off-by: Patrick McHardy <redacted>
Ok, I applied this.
While reviewing I thought it may be an issue that the new macros
potentially change skb. It really isn't an issue because NF_HOOK()
calls pass ownership of the SKB over from the caller.
Although technically, someone could go:
skb_get(skb);
err = NF_HOOK(... skb ...);
... do stuff with skb ...
kfree_skb(skb);
but that would cause other problems and I audited the entire tree
and nobody attempts anything like this currently. 'skb' always
dies at the NF_HOOK() call site.
I guess if we wanted to preserve NF_HOOK*() semantics even in such
a case we could use a local "__skb" var in the macro's basic block.
Another huge downside to this change I was worried about
was from a code generation point of view. Since we now take the
address of "skb", gcc cannot generate tail-calls for the common
case of:
return NF_HOOK(...);
when netfilter is enabled. Ho hum...
Wait...
This is actually an important point! Since gcc is generating a tail-
call for NF_HOOK() today, there is no stack savings for NF_HOOK()
created by this patch. The only real gain is the NF_STOP stuff
for bridge netfilter.
I'm backing this out of my tree, let's think about this some more.
Perhaps it's only worth adding the NF_STOP thing and just making
nf_hook_slow() do the okfn(skb); call in that case?
From: Patrick McHardy <hidden> Date: 2005-01-27 17:50:50
David S. Miller wrote:
While reviewing I thought it may be an issue that the new macros
potentially change skb. It really isn't an issue because NF_HOOK()
calls pass ownership of the SKB over from the caller.
Although technically, someone could go:
skb_get(skb);
err = NF_HOOK(... skb ...);
... do stuff with skb ...
kfree_skb(skb);
but that would cause other problems and I audited the entire tree
and nobody attempts anything like this currently. 'skb' always
dies at the NF_HOOK() call site.
Yes, it has always been illegal to use the skb after NF_HOOK.
Another huge downside to this change I was worried about
was from a code generation point of view. Since we now take the
address of "skb", gcc cannot generate tail-calls for the common
case of:
return NF_HOOK(...);
when netfilter is enabled. Ho hum...
From what I can see it doesn't generate tail-calls currently:
34c: 45 31 c0 xor %r8d,%r8d
34f: 4c 89 e2 mov %r12,%rdx
352: be 01 00 00 00 mov $0x1,%esi
357: bf 02 00 00 00 mov $0x2,%edi
35c: c7 04 24 00 00 00 80 movl $0x80000000,(%rsp)
363: e8 00 00 00 00 callq 368 <ip_local_deliver+0x248>
364: R_X86_64_PC32
nf_hook_slow+0xfffffffffffffffc
368: 48 83 c4 10 add $0x10,%rsp
36c: 5b pop %rbx
36d: 5d pop %rbp
36e: 41 5c pop %r12
370: c3 retq
According to something I found on the internet, gcc only optimizes
tail-calls if some conditions are met, in this case most importantly
the space required for the arguments to the function called at the tail
must not exceed the space required for the arguments of the function
itself. nf_hook_slow takes 6 arguments, probably more than any caller.
Regards
Patrick
From: "David S. Miller" <davem@davemloft.net> Date: 2005-01-27 19:47:26
On Thu, 27 Jan 2005 18:50:50 +0100
Patrick McHardy [off-list ref] wrote:
From what I can see it doesn't generate tail-calls currently:
Indeed... It even doesn't do this on Sparc64 either, even for
the okfn(skb) call which I was sure it would.
It won't tail-call for function pointers for some strance reason
as exhibited by this simple test:
struct sk_buff {
int foo;
};
int invoke(struct sk_buff *skb, int (*okfn)(struct sk_buff *))
{
return okfn(skb);
}
extern int test_func(struct sk_buff *);
int invoke2(struct sk_buff *skb)
{
return test_func(skb);
}
In the generated asm on sparc64, invoke2() gets a tail-call
whereas invoke() does not. Hmmm...
From: Bart De Schuymer <hidden> Date: 2005-01-27 21:16:04
Op do, 27-01-2005 te 11:47 -0800, schreef David S. Miller:
On Thu, 27 Jan 2005 18:50:50 +0100
Patrick McHardy [off-list ref] wrote:
quoted
From what I can see it doesn't generate tail-calls currently:
Indeed... It even doesn't do this on Sparc64 either, even for
the okfn(skb) call which I was sure it would.
It won't tail-call for function pointers for some strance reason
as exhibited by this simple test:
struct sk_buff {
int foo;
};
int invoke(struct sk_buff *skb, int (*okfn)(struct sk_buff *))
{
return okfn(skb);
}
extern int test_func(struct sk_buff *);
int invoke2(struct sk_buff *skb)
{
return test_func(skb);
}
In the generated asm on sparc64, invoke2() gets a tail-call
whereas invoke() does not. Hmmm...
Pasha ([off-list ref]) is currently using a bridge-nf patch vs
2.4.29 with the changes I sent to you. After two days he sent me
(yesterday) a message that all is well. Without the patch he was getting
the stack overflow oopses.
cheers,
Bart
From: "David S. Miller" <davem@davemloft.net> Date: 2005-01-27 23:24:50
On Thu, 27 Jan 2005 23:48:04 +0100
Patrick McHardy [off-list ref] wrote:
David S. Miller wrote:
quoted
In the generated asm on sparc64, invoke2() gets a tail-call
whereas invoke() does not. Hmmm...
Apparently support for indirect tail-calls was added to gcc 3.4.
Good data point.
I've been trying to figure out ways to decrease the number of
args that get sent to nf_hook_slow but this would require
some API changes unfortunately.
One idea goes like this, we create little descriptors of the form:
struct nf_hook_desc {
int (*okfn)(struct sk_buff *);
int pf;
int hook;
};
Then NF_HOOK*() callsites do something like this:
static const struct nf_hook_desc nf_ip_local_out = {
.okfn = dst_output,
.pf = PF_INET,
.hook = NF_IP_LOCAL_OUT,
};
...
/* Send it out. */
return NF_HOOK(&nf_ip_local_out, skb, NULL, rt->u.dst.dev);
This gets us down to 4 arguments from 6. I think we can kill
one more.
It is never the case that both indev and outdev are both
set, so we can use some nf_hook_desc piece of state to
indicate which (in or out) the passed device pointer is.
Oh yes, we can nicely add the thresh thing in here too
while we're at it.
So the final nf_hook_desc might look something something like:
struct nf_hook_desc {
int (*okfn)(struct sk_buff *);
int hook;
int thresh;
u8 pf; /* AF_MAX is 32 */
u8 is_output;
};
Hook could possibly use a smaller type as well to condense
the size of this thing even further. I don't know if there
are any nice assumptions we can make about the hook numbers.
Now, back to the compatability issue. We could create a
new macro, NF_HOOK_DESC() and keep the existing ones around
via some nf_hook_slow() that basically does:
int nf_hook_slow(int pf, unsigned int hook, struct sk_buff *skb,
struct net_device *indev, struct net_device *outdev,
int (*okfn)(struct sk_buff *), int thresh)
{
struct nf_hook_desc desc;
desc.okfn = okfn;
desc.hook = hook;
desc.thresh = thresh;
desc.pf = pf;
desc.is_output = (outdev != NULL);
return nf_hook_desc(&desc, skb, (outdev ? outdev : indev));
}
So the final new stuff looks something like:
#ifdef CONFIG_NETFILTER
struct nf_hook_desc {
int (*okfn)(struct sk_buff *);
int hook;
int thresh;
u8 pf; /* AF_MAX is 32 */
u8 is_output;
};
#define NF_DESC_DECLARE(_name, _okfn, _hook, _thresh, _pf, _is_output) \
static const struct nf_hook_desc _name = { \
.okfn = _okfn, \
.hook = _hook, \
.thresh = _thresh, \
.pf = _pf, \
.is_output = _is_output, \
};
extern int nf_hook_desc(struct nf_hook_desc *desc, struct sk_buff *skb,
struct net_device *dev);
#define NF_HOOK_DESC(_desc, _skb, _dev) \
nf_hook_desc(_desc, _skb, _dev)
#endif
Just throwing around ideas... comments?
From: Patrick McHardy <hidden> Date: 2005-01-28 00:08:54
David S. Miller wrote:
I've been trying to figure out ways to decrease the number of
args that get sent to nf_hook_slow but this would require
some API changes unfortunately.
One idea goes like this, we create little descriptors of the form:
struct nf_hook_desc {
int (*okfn)(struct sk_buff *);
int pf;
int hook;
};
Then NF_HOOK*() callsites do something like this:
static const struct nf_hook_desc nf_ip_local_out = {
.okfn = dst_output,
.pf = PF_INET,
.hook = NF_IP_LOCAL_OUT,
};
...
/* Send it out. */
return NF_HOOK(&nf_ip_local_out, skb, NULL, rt->u.dst.dev);
This gets us down to 4 arguments from 6. I think we can kill
one more.
It is never the case that both indev and outdev are both
set, so we can use some nf_hook_desc piece of state to
indicate which (in or out) the passed device pointer is.
indev and outdev are both set in the forward hook.
Oh yes, we can nicely add the thresh thing in here too
while we're at it.
So the final nf_hook_desc might look something something like:
struct nf_hook_desc {
int (*okfn)(struct sk_buff *);
int hook;
int thresh;
u8 pf; /* AF_MAX is 32 */
u8 is_output;
};
Hook could possibly use a smaller type as well to condense
the size of this thing even further. I don't know if there
are any nice assumptions we can make about the hook numbers.
There are currently five hooks. I really hope we'll never reach
256, so u8 should be big enough.
Now, back to the compatability issue. We could create a
new macro, NF_HOOK_DESC() and keep the existing ones around
via some nf_hook_slow() that basically does:
int nf_hook_slow(int pf, unsigned int hook, struct sk_buff *skb,
struct net_device *indev, struct net_device *outdev,
int (*okfn)(struct sk_buff *), int thresh)
{
struct nf_hook_desc desc;
desc.okfn = okfn;
desc.hook = hook;
desc.thresh = thresh;
desc.pf = pf;
desc.is_output = (outdev != NULL);
return nf_hook_desc(&desc, skb, (outdev ? outdev : indev));
}
So the final new stuff looks something like:
#ifdef CONFIG_NETFILTER
struct nf_hook_desc {
int (*okfn)(struct sk_buff *);
int hook;
int thresh;
u8 pf; /* AF_MAX is 32 */
u8 is_output;
};
#define NF_DESC_DECLARE(_name, _okfn, _hook, _thresh, _pf, _is_output) \
static const struct nf_hook_desc _name = { \
.okfn = _okfn, \
.hook = _hook, \
.thresh = _thresh, \
.pf = _pf, \
.is_output = _is_output, \
};
extern int nf_hook_desc(struct nf_hook_desc *desc, struct sk_buff *skb,
struct net_device *dev);
#define NF_HOOK_DESC(_desc, _skb, _dev) \
nf_hook_desc(_desc, _skb, _dev)
#endif
Just throwing around ideas... comments?
Sounds like a good idea to get rid of the static arguments to
nf_hook_slow. Keeping both devices we are still down from 7 to
4 arguments with your suggestion.
Regards
Patrick
From: Rusty Russell <hidden> Date: 2005-01-28 00:29:29
On Thu, 2005-01-27 at 15:24 -0800, David S. Miller wrote:
It is never the case that both indev and outdev are both
set, so we can use some nf_hook_desc piece of state to
indicate which (in or out) the passed device pointer is.
NF_IP_FORWARD... it's really *really* useful for filtering forwarded
packets. However, is it ever the case that indev != skb->dev? If not,
we can simply drop that arg and use skb->dev.
Now, back to the compatability issue. We could create a
new macro, NF_HOOK_DESC() and keep the existing ones around
via some nf_hook_slow() that basically does:
No, let's just fix them all. Also, gcc 3.4 will discard unused static
variables, so I prefer the kernel start just declaring structs as
normal, and have them "used" in the !CONFIG case by an inline function
which allows gcc to realize that it can be eliminated, but suppresses
unused warnings. This also means you get type checking etc without the
config option.
BTW, someone should go through and start ripping out #ifdef
CONFIG_PROC_FS around those structs, too...
Cheers,
Rusty.
--
A bad analogy is like a leaky screwdriver -- Richard Braakman
From: Rusty Russell <hidden> Date: 2005-01-28 01:32:25
On Thu, 2005-01-27 at 17:10 -0800, David S. Miller wrote:
On Fri, 28 Jan 2005 11:29:29 +1100
Rusty Russell [off-list ref] wrote:
quoted
No, let's just fix them all.
In tree, yes. But leaving the NF_HOOK()/NF_HOOK_THRESH() compat
macros in there for out-of-tree modules I feel is mandatory, it's
a major API change.
I'm not so sure. The hook functions which are registered, sure (ie.
keep the calling convention the same). But do any external modules use
NF_HOOK()? That implies they're writing their own network stack for
some protocol, which I would expect to be uncommon.
Rusty.
--
A bad analogy is like a leaky screwdriver -- Richard Braakman
From: Patrick McHardy <hidden> Date: 2005-01-28 01:35:42
Rusty Russell wrote:
On Thu, 2005-01-27 at 17:10 -0800, David S. Miller wrote:
quoted
In tree, yes. But leaving the NF_HOOK()/NF_HOOK_THRESH() compat
macros in there for out-of-tree modules I feel is mandatory, it's
a major API change.
I'm not so sure. The hook functions which are registered, sure (ie.
keep the calling convention the same). But do any external modules use
NF_HOOK()? That implies they're writing their own network stack for
some protocol, which I would expect to be uncommon.
Freeswan comes to mind. But I guess one more #ifdef can't hurt :)
Regards
Patrick