[PATCH 0/6] netfilter fixes for 3.3-rc6

[PATCH 0/6] netfilter fixes for 3.3-rc6

[pablo]

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

[PATCH 1/6] netfilter: ebtables: fix wrong name length while copying to user-space

[pablo]

From: Santosh Nayak <santoshprasadnayak@gmail.com>

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 <santoshprasadnayak@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/bridge/netfilter/ebtables.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 8aa4ad0..15e9575 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1335,7 +1335,12 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)m - base);
-	if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
+	char name[EBT_FUNCTION_MAXNAMELEN] = {};
+
+	/* ebtables expects 32 bytes long names but xt_match names are 29 bytes
+	   long. Copy 29 bytes and fill remaining bytes with zeroes. */
+	strncpy(name, m->u.match->name, sizeof(name));
+	if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
@@ -1344,7 +1349,10 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)w - base);
-	if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
+	char name[EBT_FUNCTION_MAXNAMELEN] = {};
+
+	strncpy(name, w->u.watcher->name, sizeof(name));
+	if (copy_to_user(hlp , name, EBT_FUNCTION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
@@ -1355,10 +1363,12 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	int ret;
 	char __user *hlp;
 	const struct ebt_entry_target *t;
+	char name[EBT_FUNCTION_MAXNAMELEN] = {};
 
 	if (e->bitmask == 0)
 		return 0;
 
+	strncpy(name, t->u.target->name, sizeof(name));
 	hlp = ubase + (((char *)e + e->target_offset) - base);
 	t = (struct ebt_entry_target *)(((char *)e) + e->target_offset);
 
@@ -1368,7 +1378,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
 	if (ret != 0)
 		return ret;
-	if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
-- 
1.7.7.3

[PATCH 2/6] netfilter: ctnetlink: remove incorrect spin_[un]lock_bh on NAT module autoload

[pablo]

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 30c9d4c..1068769 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1041,16 +1041,13 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,
 	if (!parse_nat_setup) {
 #ifdef CONFIG_MODULES
 		rcu_read_unlock();
-		spin_unlock_bh(&nf_conntrack_lock);
 		nfnl_unlock();
 		if (request_module("nf-nat-ipv4") < 0) {
 			nfnl_lock();
-			spin_lock_bh(&nf_conntrack_lock);
 			rcu_read_lock();
 			return -EOPNOTSUPP;
 		}
 		nfnl_lock();
-		spin_lock_bh(&nf_conntrack_lock);
 		rcu_read_lock();
 		if (nfnetlink_parse_nat_setup_hook)
 			return -EAGAIN;
-- 
1.7.7.3

[PATCH 3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

[pablo]

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 1068769..867843f 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1002,7 +1002,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
 	ct = nf_ct_tuplehash_to_ctrack(h);
 
 	err = -ENOMEM;
-	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 	if (skb2 == NULL) {
 		nf_ct_put(ct);
 		return -ENOMEM;
@@ -1865,7 +1865,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
 	}
 
 	err = -ENOMEM;
-	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 	if (skb2 == NULL) {
 		nf_ct_expect_put(exp);
 		goto out;
-- 
1.7.7.3

[PATCH 4/6] netfilter: bridge: fix wrong pointer dereference

[pablo]

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

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 15e9575..5fe2ff3 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1368,7 +1368,6 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	if (e->bitmask == 0)
 		return 0;
 
-	strncpy(name, t->u.target->name, sizeof(name));
 	hlp = ubase + (((char *)e + e->target_offset) - base);
 	t = (struct ebt_entry_target *)(((char *)e) + e->target_offset);
 
@@ -1378,6 +1377,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
 	if (ret != 0)
 		return ret;
+	strncpy(name, t->u.target->name, sizeof(name));
 	if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
-- 
1.7.7.3

[PATCH 5/6] bridge: netfilter: don't call iptables on vlan packets if sysctl is off

[pablo]

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

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 8412247..dec4f38 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -62,6 +62,15 @@ static int brnf_filter_pppoe_tagged __read_mostly = 0;
 #define brnf_filter_pppoe_tagged 0
 #endif
 
+#define IS_IP(skb) \
+	(!vlan_tx_tag_present(skb) && skb->protocol == htons(ETH_P_IP))
+
+#define IS_IPV6(skb) \
+	(!vlan_tx_tag_present(skb) && skb->protocol == htons(ETH_P_IPV6))
+
+#define IS_ARP(skb) \
+	(!vlan_tx_tag_present(skb) && skb->protocol == htons(ETH_P_ARP))
+
 static inline __be16 vlan_proto(const struct sk_buff *skb)
 {
 	if (vlan_tx_tag_present(skb))
@@ -639,8 +648,7 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 		return NF_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)
 			return NF_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)
 		return NF_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))
 		return NF_ACCEPT;
 
 	nf_bridge_pull_encap_header_rcsum(skb);
@@ -701,7 +708,7 @@ static int br_nf_forward_finish(struct sk_buff *skb)
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
 	struct net_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)
 	return 0;
 }
 
+
 /* This is the 'purely bridged' case.  For IP, we pass the packet to
  * netfilter with indev and outdev set to the bridge device,
  * but we are still able to filter on the 'real' indev/outdev
@@ -744,11 +752,9 @@ static unsigned int br_nf_forward_ip(unsigned int hook, struct sk_buff *skb,
 	if (!parent)
 		return NF_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;
-	else if (skb->protocol == htons(ETH_P_IPV6) || IS_VLAN_IPV6(skb) ||
-		 IS_PPPOE_IPV6(skb))
+	else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb))
 		pf = PF_INET6;
 	else
 		return NF_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)
 		return NF_ACCEPT;
 
-	if (skb->protocol != htons(ETH_P_ARP)) {
+	if (!IS_ARP(skb)) {
 		if (!IS_VLAN_ARP(skb))
 			return NF_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)
 		return NF_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;
-	else if (skb->protocol == htons(ETH_P_IPV6) || IS_VLAN_IPV6(skb) ||
-		 IS_PPPOE_IPV6(skb))
+	else if (IS_IPV6(skb) || IS_VLAN_IPV6(skb) || IS_PPPOE_IPV6(skb))
 		pf = PF_INET6;
 	else
 		return NF_ACCEPT;
-- 
1.7.7.3

[PATCH 6/6] netfilter: nf_conntrack: fix early_drop with reliable event delivery

[pablo]

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 <kerframil@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_core.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index ed86a3b..fa4b82c 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -635,8 +635,12 @@ static noinline int early_drop(struct net *net, unsigned int hash)
 
 	if (del_timer(&ct->timeout)) {
 		death_by_timeout((unsigned long)ct);
-		dropped = 1;
-		NF_CT_STAT_INC_ATOMIC(net, early_drop);
+		/* 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)) {
+			dropped = 1;
+			NF_CT_STAT_INC_ATOMIC(net, early_drop);
+		}
 	}
 	nf_ct_put(ct);
 	return dropped;
-- 
1.7.7.3

Re: [PATCH 3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

[Eric Dumazet]

On Tue, 2012-03-06 at 12:22 +0100, pablo@netfilter.org wrote:
> 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(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 1068769..867843f 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1002,7 +1002,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
>  	ct = nf_ct_tuplehash_to_ctrack(h);
>  
>  	err = -ENOMEM;
> -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>  	if (skb2 == NULL) {
>  		nf_ct_put(ct);
>  		return -ENOMEM;
> @@ -1865,7 +1865,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
>  	}
>  
>  	err = -ENOMEM;
> -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>  	if (skb2 == NULL) {
>  		nf_ct_expect_put(exp);
>  		goto out;

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.

Re: [PATCH 3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

[Pablo Neira Ayuso]

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:
> > 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(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index 1068769..867843f 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -1002,7 +1002,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
> >  	ct = nf_ct_tuplehash_to_ctrack(h);
> >  
> >  	err = -ENOMEM;
> > -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> >  	if (skb2 == NULL) {
> >  		nf_ct_put(ct);
> >  		return -ENOMEM;
> > @@ -1865,7 +1865,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
> >  	}
> >  
> >  	err = -ENOMEM;
> > -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> >  	if (skb2 == NULL) {
> >  		nf_ct_expect_put(exp);
> >  		goto out;
> 
> 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.

Re: [PATCH 3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

[Eric Dumazet]

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:

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

Re: [PATCH 3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

[Pablo Neira Ayuso]

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 :
> > On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:
> 
> > > 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.

Re: [PATCH 1/6] netfilter: ebtables: fix wrong name length while copying to user-space

[David Miller]

From: pablo@netfilter.org
Date: Tue,  6 Mar 2012 12:22:50 +0100

> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> 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 <santoshprasadnayak@gmail.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied.

Re: [PATCH 2/6] netfilter: ctnetlink: remove incorrect spin_[un]lock_bh on NAT module autoload

[David Miller]

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>

Applied.

Re: [PATCH 4/6] netfilter: bridge: fix wrong pointer dereference

[David Miller]

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>

Applied.

Re: [PATCH 5/6] bridge: netfilter: don't call iptables on vlan packets if sysctl is off

[David Miller]

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>

Applied.

Re: [PATCH 6/6] netfilter: nf_conntrack: fix early_drop with reliable event delivery

[David Miller]

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 <kerframil@gmail.com>
> 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.
		 */

Re: [PATCH 3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 6 Mar 2012 15:48:38 +0100

> David, can you take all patches except this one?

That's what I did, thanks.

Re: [PATCH 6/6] netfilter: nf_conntrack: fix early_drop with reliable event delivery

[Pablo Neira Ayuso]

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
> 
> > 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 <kerframil@gmail.com>
> > 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.
> 		 */

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