From: Pablo Neira Ayuso <pablo@netfilter.org>
Hi David,
The following patchset contains netfilter fixes for 3.3-rc6:
* one fix from Florian Westphal to fix vlan 802.1Q and netfilter bridging.
* one fix from Santosh Nayak to avoid leaking 3 bytes of data to
user-space via ebtables.
* four fixes from myself, mostly for ctnetlink. Two of them are relative
fixes to Santosh's patch (it contained one mistake) and another to
previous Jozsef's patch.
You can pull this changes from:
git://1984.lsi.us.es/net master
Netfilter updates for net-next will follow once you pull this. I won't
delay, I guess there's no much remaining time to close the merge window.
Thanks!
Florian Westphal (1):
bridge: netfilter: don't call iptables on vlan packets if sysctl is off
Pablo Neira Ayuso (4):
netfilter: ctnetlink: remove incorrect spin_[un]lock_bh on NAT module autoload
netfilter: ctnetlink: use GFP_ATOMIC in all allocations
netfilter: bridge: fix wrong pointer dereference
netfilter: nf_conntrack: fix early_drop with reliable event delivery
Santosh Nayak (1):
netfilter: ebtables: fix wrong name length while copying to user-space
net/bridge/br_netfilter.c | 32 ++++++++++++++++++--------------
net/bridge/netfilter/ebtables.c | 16 +++++++++++++---
net/netfilter/nf_conntrack_core.c | 8 ++++++--
net/netfilter/nf_conntrack_netlink.c | 7 ++-----
4 files changed, 39 insertions(+), 24 deletions(-)
--
1.7.7.3
From: Pablo Neira Ayuso <pablo@netfilter.org>
All ctnetlink operations are invoked inside rcu_read_lock
(see net/netfilter/nfnetlink.c).
Allocations have to be atomic, as RCU requires.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_netlink.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
From: Pablo Neira Ayuso <pablo@netfilter.org>
In adf7ff8, a invalid dereference was added in ebt_make_names.
CC [M] net/bridge/netfilter/ebtables.o
net/bridge/netfilter/ebtables.c: In function `ebt_make_names':
net/bridge/netfilter/ebtables.c:1371:20: warning: `t' may be used uninitialized in this function [-Wuninitialized]
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/bridge/netfilter/ebtables.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
From: Pablo Neira Ayuso <pablo@netfilter.org>
If reliable event delivery is enabled and ctnetlink fails to deliver
the destroy event in early_drop, the conntrack subsystem cannot
drop any the candidate flow that was planned to be evicted.
Reported-by: Kerin Millar <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_core.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
@@ -635,8 +635,12 @@ static noinline int early_drop(struct net *net, unsigned int hash)if(del_timer(&ct->timeout)){death_by_timeout((unsignedlong)ct);-dropped=1;-NF_CT_STAT_INC_ATOMIC(net,early_drop);+/* Check if we indeed killed this entry. Reliable event+deliverymayhaveinserteditintothedyinglist.*/+if(test_bit(IPS_DYING_BIT,&ct->status)){+dropped=1;+NF_CT_STAT_INC_ATOMIC(net,early_drop);+}}nf_ct_put(ct);returndropped;
From: Santosh Nayak <redacted>
user-space ebtables expects 32 bytes-long names, but xt_match names
use 29 bytes. We have to copy less 29 bytes and then, make sure we
fill the remaining bytes with zeroes.
Signed-off-by: Santosh Nayak <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/bridge/netfilter/ebtables.c | 16 +++++++++++++---
1 files changed, 13 insertions(+), 3 deletions(-)
From: Pablo Neira Ayuso <pablo@netfilter.org>
Since 7d367e0, ctnetlink_new_conntrack is called without holding
the nf_conntrack_lock spinlock. Thus, ctnetlink_parse_nat_setup
does not require to release that spinlock anymore in the NAT module
autoload case.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_netlink.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
From: Florian Westphal <fw@strlen.de>
When net.bridge.bridge-nf-filter-vlan-tagged is 0 (default), vlan packets
arriving should not be sent to ip(6)tables by bridge netfilter.
However, it turns out that we currently always send VLAN packets to
netfilter, if ..
a), CONFIG_VLAN_8021Q is enabled ; or
b), CONFIG_VLAN_8021Q is not set but rx vlan offload is enabled
on the bridge port.
This is because bridge netfilter treats skb with
skb->protocol == ETH_P_IP{V6} as "non-vlan packet".
With rx vlan offload on or CONFIG_VLAN_8021Q=y, the vlan header has
already been removed here, and we cannot rely on skb->protocol alone.
Fix this by only using skb->protocol if the skb has no vlan tag,
or if a vlan tag is present and filter-vlan-tagged bridge netfilter
sysctl is enabled.
We cannot remove the skb->protocol == htons(ETH_P_8021Q) test
because the vlan tag is still around in the CONFIG_VLAN_8021Q=n &&
"ethtool -K $itf rxvlan off" case.
reproducer:
iptables -t raw -I PREROUTING -i br0
iptables -t raw -I PREROUTING -i br0.1
Then send packets to an ip address configured on br0.1 interface.
Even with net.bridge.bridge-nf-filter-vlan-tagged=0, the 1st rule
will match instead of the 2nd one.
With this patch applied, the 2nd rule will match instead.
In the non-local address case, netfilter won't be consulted after
this patch unless the sysctl is switched on.
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/bridge/br_netfilter.c | 32 ++++++++++++++++++--------------
1 files changed, 18 insertions(+), 14 deletions(-)
@@ -639,8 +648,7 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,returnNF_DROP;br=p->br;-if(skb->protocol==htons(ETH_P_IPV6)||IS_VLAN_IPV6(skb)||-IS_PPPOE_IPV6(skb)){+if(IS_IPV6(skb)||IS_VLAN_IPV6(skb)||IS_PPPOE_IPV6(skb)){if(!brnf_call_ip6tables&&!br->nf_call_ip6tables)returnNF_ACCEPT;
@@ -651,8 +659,7 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,if(!brnf_call_iptables&&!br->nf_call_iptables)returnNF_ACCEPT;-if(skb->protocol!=htons(ETH_P_IP)&&!IS_VLAN_IP(skb)&&-!IS_PPPOE_IP(skb))+if(!IS_IP(skb)&&!IS_VLAN_IP(skb)&&!IS_PPPOE_IP(skb))returnNF_ACCEPT;nf_bridge_pull_encap_header_rcsum(skb);
@@ -701,7 +708,7 @@ static int br_nf_forward_finish(struct sk_buff *skb)structnf_bridge_info*nf_bridge=skb->nf_bridge;structnet_device*in;-if(skb->protocol!=htons(ETH_P_ARP)&&!IS_VLAN_ARP(skb)){+if(!IS_ARP(skb)&&!IS_VLAN_ARP(skb)){in=nf_bridge->physindev;if(nf_bridge->mask&BRNF_PKT_TYPE){skb->pkt_type=PACKET_OTHERHOST;
@@ -718,6 +725,7 @@ static int br_nf_forward_finish(struct sk_buff *skb)return0;}+/* This is the 'purely bridged' case. For IP, we pass the packet to*netfilterwithindevandoutdevsettothebridgedevice,*butwearestillabletofilteronthe'real'indev/outdev
@@ -744,11 +752,9 @@ static unsigned int br_nf_forward_ip(unsigned int hook, struct sk_buff *skb,if(!parent)returnNF_DROP;-if(skb->protocol==htons(ETH_P_IP)||IS_VLAN_IP(skb)||-IS_PPPOE_IP(skb))+if(IS_IP(skb)||IS_VLAN_IP(skb)||IS_PPPOE_IP(skb))pf=PF_INET;-elseif(skb->protocol==htons(ETH_P_IPV6)||IS_VLAN_IPV6(skb)||-IS_PPPOE_IPV6(skb))+elseif(IS_IPV6(skb)||IS_VLAN_IPV6(skb)||IS_PPPOE_IPV6(skb))pf=PF_INET6;elsereturnNF_ACCEPT;
@@ -795,7 +801,7 @@ static unsigned int br_nf_forward_arp(unsigned int hook, struct sk_buff *skb,if(!brnf_call_arptables&&!br->nf_call_arptables)returnNF_ACCEPT;-if(skb->protocol!=htons(ETH_P_ARP)){+if(!IS_ARP(skb)){if(!IS_VLAN_ARP(skb))returnNF_ACCEPT;nf_bridge_pull_encap_header(skb);
@@ -853,11 +859,9 @@ static unsigned int br_nf_post_routing(unsigned int hook, struct sk_buff *skb,if(!realoutdev)returnNF_DROP;-if(skb->protocol==htons(ETH_P_IP)||IS_VLAN_IP(skb)||-IS_PPPOE_IP(skb))+if(IS_IP(skb)||IS_VLAN_IP(skb)||IS_PPPOE_IP(skb))pf=PF_INET;-elseif(skb->protocol==htons(ETH_P_IPV6)||IS_VLAN_IPV6(skb)||-IS_PPPOE_IPV6(skb))+elseif(IS_IPV6(skb)||IS_VLAN_IPV6(skb)||IS_PPPOE_IPV6(skb))pf=PF_INET6;elsereturnNF_ACCEPT;
From: Eric Dumazet <hidden> Date: 2012-03-06 12:50:21
On Tue, 2012-03-06 at 12:22 +0100, pablo@netfilter.org wrote:
quoted hunk
From: Pablo Neira Ayuso <pablo@netfilter.org>
All ctnetlink operations are invoked inside rcu_read_lock
(see net/netfilter/nfnetlink.c).
Allocations have to be atomic, as RCU requires.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_netlink.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
This cant be right.
Really this must be kept as GFP_KERNEL allocations.
Only if .call_rcu member is used in place of .call rcu_read_lock() is
held instead of nfnl_lock().
You should take a look at all GFP_ATOMIC uses in
net/netfilter/nf_conntrack_netlink.c and check if they can be GFP_KERNEL
instead.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2012-03-06 14:48:38
On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:
On Tue, 2012-03-06 at 12:22 +0100, pablo@netfilter.org wrote:
quoted
From: Pablo Neira Ayuso <pablo@netfilter.org>
All ctnetlink operations are invoked inside rcu_read_lock
(see net/netfilter/nfnetlink.c).
Allocations have to be atomic, as RCU requires.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_netlink.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
This cant be right.
Really this must be kept as GFP_KERNEL allocations.
Only if .call_rcu member is used in place of .call rcu_read_lock() is
held instead of nfnl_lock().
I thought we couldn't sleep while holding rcu_read_lock.
You should take a look at all GFP_ATOMIC uses in
net/netfilter/nf_conntrack_netlink.c and check if they can be GFP_KERNEL
instead.
David, can you take all patches except this one?
I'll have to rebase my tree after this, sorry.
From: Eric Dumazet <hidden> Date: 2012-03-06 15:09:17
Le mardi 06 mars 2012 à 15:48 +0100, Pablo Neira Ayuso a écrit :
On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:
quoted
This cant be right.
Really this must be kept as GFP_KERNEL allocations.
Only if .call_rcu member is used in place of .call rcu_read_lock() is
held instead of nfnl_lock().
I thought we couldn't sleep while holding rcu_read_lock.
True, but as far as I can see we dont hold rcu_read_lock() at this
point, only a mutex.
I added the .call_rcu() mechanism in struct nfnl_callback only for very
specific needs, namely performance improvements in commit 84a797dd0
(netfilter: nfnetlink_queue: provide rcu enabled callbacks)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2012-03-06 15:35:18
On Tue, Mar 06, 2012 at 07:09:17AM -0800, Eric Dumazet wrote:
Le mardi 06 mars 2012 à 15:48 +0100, Pablo Neira Ayuso a écrit :
quoted
On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:
quoted
quoted
This cant be right.
Really this must be kept as GFP_KERNEL allocations.
Only if .call_rcu member is used in place of .call rcu_read_lock() is
held instead of nfnl_lock().
I thought we couldn't sleep while holding rcu_read_lock.
True, but as far as I can see we dont hold rcu_read_lock() at this
point, only a mutex.
I added the .call_rcu() mechanism in struct nfnl_callback only for very
specific needs, namely performance improvements in commit 84a797dd0
(netfilter: nfnetlink_queue: provide rcu enabled callbacks)
Sorry, I overlooked that changed, I still thought that we were calling
these under rcu_read_lock.
This patch has to be kept out indeed. Thanks for spotting this Eric.
From: David Miller <davem@davemloft.net> Date: 2012-03-06 20:15:20
From: pablo@netfilter.org
Date: Tue, 6 Mar 2012 12:22:51 +0100
From: Pablo Neira Ayuso <pablo@netfilter.org>
Since 7d367e0, ctnetlink_new_conntrack is called without holding
the nf_conntrack_lock spinlock. Thus, ctnetlink_parse_nat_setup
does not require to release that spinlock anymore in the NAT module
autoload case.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
From: David Miller <davem@davemloft.net> Date: 2012-03-06 20:15:27
From: pablo@netfilter.org
Date: Tue, 6 Mar 2012 12:22:53 +0100
From: Pablo Neira Ayuso <pablo@netfilter.org>
In adf7ff8, a invalid dereference was added in ebt_make_names.
CC [M] net/bridge/netfilter/ebtables.o
net/bridge/netfilter/ebtables.c: In function `ebt_make_names':
net/bridge/netfilter/ebtables.c:1371:20: warning: `t' may be used uninitialized in this function [-Wuninitialized]
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
From: David Miller <davem@davemloft.net> Date: 2012-03-06 20:16:17
From: pablo@netfilter.org
Date: Tue, 6 Mar 2012 12:22:50 +0100
From: Santosh Nayak <redacted>
user-space ebtables expects 32 bytes-long names, but xt_match names
use 29 bytes. We have to copy less 29 bytes and then, make sure we
fill the remaining bytes with zeroes.
Signed-off-by: Santosh Nayak <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
From: David Miller <davem@davemloft.net> Date: 2012-03-06 20:16:18
From: pablo@netfilter.org
Date: Tue, 6 Mar 2012 12:22:55 +0100
From: Pablo Neira Ayuso <pablo@netfilter.org>
If reliable event delivery is enabled and ctnetlink fails to deliver
the destroy event in early_drop, the conntrack subsystem cannot
drop any the candidate flow that was planned to be evicted.
Reported-by: Kerin Millar <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Applied.
+ /* Check if we indeed killed this entry. Reliable event
+ delivery may have inserted it into the dying list. */
+ if (test_bit(IPS_DYING_BIT, &ct->status)) {
Please don't allow misformatted comments like this into your
tree next time, this should be:
/* Check if we indeed killed this entry. Reliable event
* delivery may have inserted it into the dying list.
*/
From: David Miller <davem@davemloft.net> Date: 2012-03-06 20:16:45
From: pablo@netfilter.org
Date: Tue, 6 Mar 2012 12:22:54 +0100
From: Florian Westphal <fw@strlen.de>
When net.bridge.bridge-nf-filter-vlan-tagged is 0 (default), vlan packets
arriving should not be sent to ip(6)tables by bridge netfilter.
...
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2012-03-07 13:19:20
On Tue, Mar 06, 2012 at 03:16:18PM -0500, David Miller wrote:
From: pablo@netfilter.org
Date: Tue, 6 Mar 2012 12:22:55 +0100
quoted
From: Pablo Neira Ayuso <pablo@netfilter.org>
If reliable event delivery is enabled and ctnetlink fails to deliver
the destroy event in early_drop, the conntrack subsystem cannot
drop any the candidate flow that was planned to be evicted.
Reported-by: Kerin Millar <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Applied.
quoted
+ /* Check if we indeed killed this entry. Reliable event
+ delivery may have inserted it into the dying list. */
+ if (test_bit(IPS_DYING_BIT, &ct->status)) {
Please don't allow misformatted comments like this into your
tree next time, this should be:
/* Check if we indeed killed this entry. Reliable event
* delivery may have inserted it into the dying list.
*/
Thanks for spotting this.
Will in the future (probably some patches that I took for net-next
may not follow this format either, but for upcoming patches I will).