netfilter -stable 00/08: netfilter -stable fixes

netfilter -stable 00/08: netfilter -stable fixes

[Patrick McHardy]

Following are couple of netfilter fixes for -stable, fixing

- various races in nf_conntrack introduced by the conversion to use
  RCU for the conntrack hash and follow-up patch to use SLAB_DESTROY_BY_RCU
  for the conntrack slab

- direct userspace memory access in the nf_log /proc handler

- a missing initialization in the quota match, possibly causing malfunction
  on SMP

- an incorrect comparison in the rateest match

- unacknowledged data detection in TCP conntrack in combination with
  NAT helpers reducing the packet size

Please apply, thanks.


 Documentation/RCU/rculist_nulls.txt    |    7 +++++-
 include/net/netfilter/nf_conntrack.h   |    4 +-
 net/ipv4/netfilter/nf_nat_helper.c     |   17 +++++++++-----
 net/netfilter/nf_conntrack_core.c      |   36 ++++++++++++++++++++++++++-----
 net/netfilter/nf_conntrack_proto_tcp.c |    6 ++--
 net/netfilter/nf_log.c                 |   22 ++++++++++++-------
 net/netfilter/xt_quota.c               |    1 +
 net/netfilter/xt_rateest.c             |    2 +-
 8 files changed, 68 insertions(+), 27 deletions(-)

Patrick McHardy (8):
      netfilter: nf_log: fix sleeping function called from invalid context
      netfilter: nf_conntrack: fix confirmation race condition
      netfilter: nf_conntrack: fix conntrack lookup race
      netfilter: nf_log: fix direct userspace memory access in proc handler
      netfilter: xt_quota: fix incomplete initialization
      netfilter: xt_rateest: fix comparison with self
      netfilter: tcp conntrack: fix unacknowledged data detection with NAT
      netfilter: nf_conntrack: nf_conntrack_alloc() fixes

netfilter -stable 01/08: nf_log: fix sleeping function called from invalid context

[Patrick McHardy]

commit c45f59bca9d15e911636f9083035f6f847cff125
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:31:15 2009 +0200

    netfilter: nf_log: fix sleeping function called from invalid context
    
    Upstream commit 266d07cb1:
    
    Fix regression introduced by 17625274 "netfilter: sysctl support of
    logger choice":
    
    BUG: sleeping function called from invalid context at /mnt/s390test/linux-2.6-tip/arch/s390/include/as
    in_atomic(): 1, irqs_disabled(): 0, pid: 3245, name: sysctl
    CPU: 1 Not tainted 2.6.30-rc8-tipjun10-02053-g39ae214 #1
    Process sysctl (pid: 3245, task: 000000007f675da0, ksp: 000000007eb17cf0)
    0000000000000000 000000007eb17be8 0000000000000002 0000000000000000
           000000007eb17c88 000000007eb17c00 000000007eb17c00 0000000000048156
           00000000003e2de8 000000007f676118 000000007eb17f10 0000000000000000
           0000000000000000 000000007eb17be8 000000000000000d 000000007eb17c58
           00000000003e2050 000000000001635c 000000007eb17be8 000000007eb17c30
    Call Trace:
    (Ý<00000000000162e6>¨ show_trace+0x13a/0x148)
     Ý<00000000000349ea>¨ __might_sleep+0x13a/0x164
     Ý<0000000000050300>¨ proc_dostring+0x134/0x22c
     Ý<0000000000312b70>¨ nf_log_proc_dostring+0xfc/0x188
     Ý<0000000000136f5e>¨ proc_sys_call_handler+0xf6/0x118
     Ý<0000000000136fda>¨ proc_sys_read+0x26/0x34
     Ý<00000000000d6e9c>¨ vfs_read+0xac/0x158
     Ý<00000000000d703e>¨ SyS_read+0x56/0x88
     Ý<0000000000027f42>¨ sysc_noemu+0x10/0x16
    
    Use the nf_log_mutex instead of RCU to fix this.
    
    Reported-and-tested-by: Maran Pakkirisamy <maranpsamy@in.ibm.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index beb3731..2fefe14 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -248,14 +248,14 @@ static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
 		rcu_assign_pointer(nf_loggers[tindex], logger);
 		mutex_unlock(&nf_log_mutex);
 	} else {
-		rcu_read_lock();
-		logger = rcu_dereference(nf_loggers[tindex]);
+		mutex_lock(&nf_log_mutex);
+		logger = nf_loggers[tindex];
 		if (!logger)
 			table->data = "NONE";
 		else
 			table->data = logger->name;
 		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
-		rcu_read_unlock();
+		mutex_unlock(&nf_log_mutex);
 	}
 
 	return r;
--
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

netfilter -stable 02/08: nf_conntrack: fix confirmation race condition

[Patrick McHardy]

commit 0293f8bc787be0f6a0ff9dba877c2f87f43dc60f
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:33:04 2009 +0200

    netfilter: nf_conntrack: fix confirmation race condition
    
    Upstream commit 5c8ec910:
    
    New connection tracking entries are inserted into the hash before they
    are fully set up, namely the CONFIRMED bit is not set and the timer not
    started yet. This can theoretically lead to a race with timer, which
    would set the timeout value to a relative value, most likely already in
    the past.
    
    Perform hash insertion as the final step to fix this.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 8020db6..c8bd559 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -385,7 +385,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	/* Remove from unconfirmed list */
 	hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
 
-	__nf_conntrack_hash_insert(ct, hash, repl_hash);
 	/* Timer relative to confirmation time, not original
 	   setting time, otherwise we'd get timer wrap in
 	   weird delay cases. */
@@ -393,8 +392,16 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	add_timer(&ct->timeout);
 	atomic_inc(&ct->ct_general.use);
 	set_bit(IPS_CONFIRMED_BIT, &ct->status);
+
+	/* Since the lookup is lockless, hash insertion must be done after
+	 * starting the timer and setting the CONFIRMED bit. The RCU barriers
+	 * guarantee that no other CPU can find the conntrack before the above
+	 * stores are visible.
+	 */
+	__nf_conntrack_hash_insert(ct, hash, repl_hash);
 	NF_CT_STAT_INC(net, insert);
 	spin_unlock_bh(&nf_conntrack_lock);
+
 	help = nfct_help(ct);
 	if (help && help->helper)
 		nf_conntrack_event_cache(IPCT_HELPER, ct);

netfilter -stable 03/08: nf_conntrack: fix conntrack lookup race

[Patrick McHardy]

commit 1c91562def07c7186c2e17fc23efef75fb4dbd14
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:33:45 2009 +0200

    netfilter: nf_conntrack: fix conntrack lookup race
    
    Upstream commit 8d8890b7:
    
    The RCU protected conntrack hash lookup only checks whether the entry
    has a refcount of zero to decide whether it is stale. This is not
    sufficient, entries are explicitly removed while there is at least
    one reference left, possibly more. Explicitly check whether the entry
    has been marked as dying to fix this.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index c8bd559..2d3584f 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -295,7 +295,8 @@ begin:
 	h = __nf_conntrack_find(net, tuple);
 	if (h) {
 		ct = nf_ct_tuplehash_to_ctrack(h);
-		if (unlikely(!atomic_inc_not_zero(&ct->ct_general.use)))
+		if (unlikely(nf_ct_is_dying(ct) ||
+			     !atomic_inc_not_zero(&ct->ct_general.use)))
 			h = NULL;
 		else {
 			if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple))) {
@@ -474,7 +475,8 @@ static noinline int early_drop(struct net *net, unsigned int hash)
 			cnt++;
 		}
 
-		if (ct && unlikely(!atomic_inc_not_zero(&ct->ct_general.use)))
+		if (ct && unlikely(nf_ct_is_dying(ct) ||
+				   !atomic_inc_not_zero(&ct->ct_general.use)))
 			ct = NULL;
 		if (ct || cnt >= NF_CT_EVICTION_RANGE)
 			break;

netfilter -stable 04/08: nf_log: fix direct userspace memory access in proc handler

[Patrick McHardy]

commit b8fe73c51f6edb0efe547765bf47fad382d9fc98
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:34:28 2009 +0200

    netfilter: nf_log: fix direct userspace memory access in proc handler
    
    Upstream commit 24955619.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 2fefe14..4e62030 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -47,7 +47,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 	mutex_lock(&nf_log_mutex);
 
 	if (pf == NFPROTO_UNSPEC) {
-		int i;
 		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
 			list_add_tail(&(logger->list[i]), &(nf_loggers_l[i]));
 	} else {
@@ -216,7 +215,7 @@ static const struct file_operations nflog_file_ops = {
 #endif /* PROC_FS */
 
 #ifdef CONFIG_SYSCTL
-struct ctl_path nf_log_sysctl_path[] = {
+static struct ctl_path nf_log_sysctl_path[] = {
 	{ .procname = "net", .ctl_name = CTL_NET, },
 	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
 	{ .procname = "nf_log", .ctl_name = CTL_UNNUMBERED, },
@@ -228,19 +227,26 @@ static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
 static struct ctl_table_header *nf_log_dir_header;
 
 static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
-			 void *buffer, size_t *lenp, loff_t *ppos)
+			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	const struct nf_logger *logger;
+	char buf[NFLOGGER_NAME_LEN];
+	size_t size = *lenp;
 	int r = 0;
 	int tindex = (unsigned long)table->extra1;
 
 	if (write) {
-		if (!strcmp(buffer, "NONE")) {
+		if (size > sizeof(buf))
+			size = sizeof(buf);
+		if (copy_from_user(buf, buffer, size))
+			return -EFAULT;
+
+		if (!strcmp(buf, "NONE")) {
 			nf_log_unbind_pf(tindex);
 			return 0;
 		}
 		mutex_lock(&nf_log_mutex);
-		logger = __find_logger(tindex, buffer);
+		logger = __find_logger(tindex, buf);
 		if (logger == NULL) {
 			mutex_unlock(&nf_log_mutex);
 			return -ENOENT;

netfilter -stable 05/08: xt_quota: fix incomplete initialization

[Patrick McHardy]

commit 60cc46b2f32e6829efa4067914da11e4d64f421a
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:35:12 2009 +0200

    netfilter: xt_quota: fix incomplete initialization
    
    Upstream commit 6d62182f:
    
    Commit v2.6.29-rc5-872-gacc738f ("xtables: avoid pointer to self")
    forgot to copy the initial quota value supplied by iptables into the
    private structure, thus counting from whatever was in the memory
    kmalloc returned.
    
    Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
index 01dd07b..98fc190 100644
--- a/net/netfilter/xt_quota.c
+++ b/net/netfilter/xt_quota.c
@@ -54,6 +54,7 @@ static bool quota_mt_check(const struct xt_mtchk_param *par)
 	if (q->master == NULL)
 		return -ENOMEM;
 
+	q->master->quota = q->quota;
 	return true;
 }
 

netfilter -stable 06/08: xt_rateest: fix comparison with self

[Patrick McHardy]

commit 77609e5aca9bc8cd4e9ea960177df1e980b3d040
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:35:55 2009 +0200

    netfilter: xt_rateest: fix comparison with self
    
    Upstream commit 4d900f9df:
    
    As noticed by Török Edwin <edwintorok@gmail.com>:
    
    Compiling the kernel with clang has shown this warning:
    
    net/netfilter/xt_rateest.c:69:16: warning: self-comparison always results in a
    constant value
                            ret &= pps2 == pps2;
                                        ^
    Looking at the code:
    if (info->flags & XT_RATEEST_MATCH_BPS)
                ret &= bps1 == bps2;
            if (info->flags & XT_RATEEST_MATCH_PPS)
                ret &= pps2 == pps2;
    
    Judging from the MATCH_BPS case it seems to be a typo, with the intention of
    comparing pps1 with pps2.
    
    http://bugzilla.kernel.org/show_bug.cgi?id=13535
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_rateest.c b/net/netfilter/xt_rateest.c
index 220a1d5..4fc6a91 100644
--- a/net/netfilter/xt_rateest.c
+++ b/net/netfilter/xt_rateest.c
@@ -66,7 +66,7 @@ xt_rateest_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 		if (info->flags & XT_RATEEST_MATCH_BPS)
 			ret &= bps1 == bps2;
 		if (info->flags & XT_RATEEST_MATCH_PPS)
-			ret &= pps2 == pps2;
+			ret &= pps1 == pps2;
 		break;
 	}
 

netfilter -stable 07/08: tcp conntrack: fix unacknowledged data detection with NAT

[Patrick McHardy]

commit e7a42c94e8d0845d10163f6c2695a8342c2ce1cd
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Jul 3 10:37:27 2009 +0200

    netfilter: tcp conntrack: fix unacknowledged data detection with NAT
    
    Upstream commit a3a9f79e:
    
    When NAT helpers change the TCP packet size, the highest seen sequence
    number needs to be corrected. This is currently only done upwards, when
    the packet size is reduced the sequence number is unchanged. This causes
    TCP conntrack to falsely detect unacknowledged data and decrease the
    timeout.
    
    Fix by updating the highest seen sequence number in both directions after
    packet mangling.
    
    Tested-by: Krzysztof Piotr Oledzki <ole@ans.pl>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 6c3f964..5d9a848 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -255,8 +255,8 @@ static inline bool nf_ct_kill(struct nf_conn *ct)
 /* Update TCP window tracking data when NAT mangles the packet */
 extern void nf_conntrack_tcp_update(const struct sk_buff *skb,
 				    unsigned int dataoff,
-				    struct nf_conn *ct,
-				    int dir);
+				    struct nf_conn *ct, int dir,
+				    s16 offset);
 
 /* Fake conntrack entry for untracked connections */
 extern struct nf_conn nf_conntrack_untracked;
diff --git a/net/ipv4/netfilter/nf_nat_helper.c b/net/ipv4/netfilter/nf_nat_helper.c
index cf7a42b..05ede41 100644
--- a/net/ipv4/netfilter/nf_nat_helper.c
+++ b/net/ipv4/netfilter/nf_nat_helper.c
@@ -191,7 +191,8 @@ nf_nat_mangle_tcp_packet(struct sk_buff *skb,
 				    ct, ctinfo);
 		/* Tell TCP window tracking about seq change */
 		nf_conntrack_tcp_update(skb, ip_hdrlen(skb),
-					ct, CTINFO2DIR(ctinfo));
+					ct, CTINFO2DIR(ctinfo),
+					(int)rep_len - (int)match_len);
 
 		nf_conntrack_event_cache(IPCT_NATSEQADJ, ct);
 	}
@@ -377,6 +378,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 	struct tcphdr *tcph;
 	int dir;
 	__be32 newseq, newack;
+	s16 seqoff, ackoff;
 	struct nf_conn_nat *nat = nfct_nat(ct);
 	struct nf_nat_seq *this_way, *other_way;
 
@@ -390,15 +392,18 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 
 	tcph = (void *)skb->data + ip_hdrlen(skb);
 	if (after(ntohl(tcph->seq), this_way->correction_pos))
-		newseq = htonl(ntohl(tcph->seq) + this_way->offset_after);
+		seqoff = this_way->offset_after;
 	else
-		newseq = htonl(ntohl(tcph->seq) + this_way->offset_before);
+		seqoff = this_way->offset_before;
 
 	if (after(ntohl(tcph->ack_seq) - other_way->offset_before,
 		  other_way->correction_pos))
-		newack = htonl(ntohl(tcph->ack_seq) - other_way->offset_after);
+		ackoff = other_way->offset_after;
 	else
-		newack = htonl(ntohl(tcph->ack_seq) - other_way->offset_before);
+		ackoff = other_way->offset_before;
+
+	newseq = htonl(ntohl(tcph->seq) + seqoff);
+	newack = htonl(ntohl(tcph->ack_seq) - ackoff);
 
 	inet_proto_csum_replace4(&tcph->check, skb, tcph->seq, newseq, 0);
 	inet_proto_csum_replace4(&tcph->check, skb, tcph->ack_seq, newack, 0);
@@ -413,7 +418,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
 	if (!nf_nat_sack_adjust(skb, tcph, ct, ctinfo))
 		return 0;
 
-	nf_conntrack_tcp_update(skb, ip_hdrlen(skb), ct, dir);
+	nf_conntrack_tcp_update(skb, ip_hdrlen(skb), ct, dir, seqoff);
 
 	return 1;
 }
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 97a6e93..a38bc22 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -706,8 +706,8 @@ static bool tcp_in_window(const struct nf_conn *ct,
 /* Caller must linearize skb at tcp header. */
 void nf_conntrack_tcp_update(const struct sk_buff *skb,
 			     unsigned int dataoff,
-			     struct nf_conn *ct,
-			     int dir)
+			     struct nf_conn *ct, int dir,
+			     s16 offset)
 {
 	const struct tcphdr *tcph = (const void *)skb->data + dataoff;
 	const struct ip_ct_tcp_state *sender = &ct->proto.tcp.seen[dir];
@@ -720,7 +720,7 @@ void nf_conntrack_tcp_update(const struct sk_buff *skb,
 	/*
 	 * We have to worry for the ack in the reply packet only...
 	 */
-	if (after(end, ct->proto.tcp.seen[dir].td_end))
+	if (ct->proto.tcp.seen[dir].td_end + offset == end)
 		ct->proto.tcp.seen[dir].td_end = end;
 	ct->proto.tcp.last_end = end;
 	write_unlock_bh(&tcp_lock);

netfilter -stable 08/08: nf_conntrack: nf_conntrack_alloc() fixes

[Patrick McHardy]

commit 2b12e9634d9676c55f576195a7f950956179d881
Author: Patrick McHardy <kaber@trash.net>
Date:   Thu Jul 23 16:02:18 2009 +0200

    netfilter: nf_conntrack: nf_conntrack_alloc() fixes
    
    Upstream commit 941297f4:
    
    When a slab cache uses SLAB_DESTROY_BY_RCU, we must be careful when allocating
    objects, since slab allocator could give a freed object still used by lockless
    readers.
    
    In particular, nf_conntrack RCU lookups rely on ct->tuplehash[xxx].hnnode.next
    being always valid (ie containing a valid 'nulls' value, or a valid pointer to next
    object in hash chain.)
    
    kmem_cache_zalloc() setups object with NULL values, but a NULL value is not valid
    for ct->tuplehash[xxx].hnnode.next.
    
    Fix is to call kmem_cache_alloc() and do the zeroing ourself.
    
    As spotted by Patrick, we also need to make sure lookup keys are committed to
    memory before setting refcount to 1, or a lockless reader could get a reference
    on the old version of the object. Its key re-check could then pass the barrier.
    
    Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/Documentation/RCU/rculist_nulls.txt b/Documentation/RCU/rculist_nulls.txt
index 6389dec..d0c017e 100644
--- a/Documentation/RCU/rculist_nulls.txt
+++ b/Documentation/RCU/rculist_nulls.txt
@@ -83,11 +83,12 @@ not detect it missed following items in original chain.
 obj = kmem_cache_alloc(...);
 lock_chain(); // typically a spin_lock()
 obj->key = key;
-atomic_inc(&obj->refcnt);
 /*
  * we need to make sure obj->key is updated before obj->next
+ * or obj->refcnt
  */
 smp_wmb();
+atomic_set(&obj->refcnt, 1);
 hlist_add_head_rcu(&obj->obj_node, list);
 unlock_chain(); // typically a spin_unlock()
 
@@ -159,6 +160,10 @@ out:
 obj = kmem_cache_alloc(cachep);
 lock_chain(); // typically a spin_lock()
 obj->key = key;
+/*
+ * changes to obj->key must be visible before refcnt one
+ */
+smp_wmb();
 atomic_set(&obj->refcnt, 1);
 /*
  * insert obj in RCU way (readers might be traversing chain)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 2d3584f..0d961ee 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -525,22 +525,37 @@ struct nf_conn *nf_conntrack_alloc(struct net *net,
 		}
 	}
 
-	ct = kmem_cache_zalloc(nf_conntrack_cachep, gfp);
+	/*
+	 * Do not use kmem_cache_zalloc(), as this cache uses
+	 * SLAB_DESTROY_BY_RCU.
+	 */
+	ct = kmem_cache_alloc(nf_conntrack_cachep, gfp);
 	if (ct == NULL) {
 		pr_debug("nf_conntrack_alloc: Can't alloc conntrack.\n");
 		atomic_dec(&net->ct.count);
 		return ERR_PTR(-ENOMEM);
 	}
-
-	atomic_set(&ct->ct_general.use, 1);
+	/*
+	 * Let ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.next
+	 * and ct->tuplehash[IP_CT_DIR_REPLY].hnnode.next unchanged.
+	 */
+	memset(&ct->tuplehash[IP_CT_DIR_MAX], 0,
+	       sizeof(*ct) - offsetof(struct nf_conn, tuplehash[IP_CT_DIR_MAX]));
 	ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple = *orig;
+	ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.pprev = NULL;
 	ct->tuplehash[IP_CT_DIR_REPLY].tuple = *repl;
+	ct->tuplehash[IP_CT_DIR_REPLY].hnnode.pprev = NULL;
 	/* Don't set timer yet: wait for confirmation */
 	setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);
 #ifdef CONFIG_NET_NS
 	ct->ct_net = net;
 #endif
 
+	/*
+	 * changes to lookup keys must be done before setting refcnt to 1
+	 */
+	smp_wmb();
+	atomic_set(&ct->ct_general.use, 1);
 	return ct;
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alloc);

Re: netfilter -stable 08/08: nf_conntrack: nf_conntrack_alloc() fixes

[Paul E. McKenney]

On Thu, Jul 23, 2009 at 04:15:34PM +0200, Patrick McHardy wrote:
> commit 2b12e9634d9676c55f576195a7f950956179d881
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Thu Jul 23 16:02:18 2009 +0200
> 
>     netfilter: nf_conntrack: nf_conntrack_alloc() fixes
>     
>     Upstream commit 941297f4:
>     
>     When a slab cache uses SLAB_DESTROY_BY_RCU, we must be careful when allocating
>     objects, since slab allocator could give a freed object still used by lockless
>     readers.
>     
>     In particular, nf_conntrack RCU lookups rely on ct->tuplehash[xxx].hnnode.next
>     being always valid (ie containing a valid 'nulls' value, or a valid pointer to next
>     object in hash chain.)
>     
>     kmem_cache_zalloc() setups object with NULL values, but a NULL value is not valid
>     for ct->tuplehash[xxx].hnnode.next.
>     
>     Fix is to call kmem_cache_alloc() and do the zeroing ourself.
>     
>     As spotted by Patrick, we also need to make sure lookup keys are committed to
>     memory before setting refcount to 1, or a lockless reader could get a reference
>     on the old version of the object. Its key re-check could then pass the barrier.

The atomic_inc() works, but only if ->refcnt is set to zero on the
initial allocation.  That said, this approach seems safer, so:

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

>     Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> diff --git a/Documentation/RCU/rculist_nulls.txt b/Documentation/RCU/rculist_nulls.txt
> index 6389dec..d0c017e 100644
> --- a/Documentation/RCU/rculist_nulls.txt
> +++ b/Documentation/RCU/rculist_nulls.txt
> @@ -83,11 +83,12 @@ not detect it missed following items in original chain.
>  obj = kmem_cache_alloc(...);
>  lock_chain(); // typically a spin_lock()
>  obj->key = key;
> -atomic_inc(&obj->refcnt);
>  /*
>   * we need to make sure obj->key is updated before obj->next
> + * or obj->refcnt
>   */
>  smp_wmb();
> +atomic_set(&obj->refcnt, 1);
>  hlist_add_head_rcu(&obj->obj_node, list);
>  unlock_chain(); // typically a spin_unlock()
> 
> @@ -159,6 +160,10 @@ out:
>  obj = kmem_cache_alloc(cachep);
>  lock_chain(); // typically a spin_lock()
>  obj->key = key;
> +/*
> + * changes to obj->key must be visible before refcnt one
> + */
> +smp_wmb();
>  atomic_set(&obj->refcnt, 1);
>  /*
>   * insert obj in RCU way (readers might be traversing chain)
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 2d3584f..0d961ee 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -525,22 +525,37 @@ struct nf_conn *nf_conntrack_alloc(struct net *net,
>  		}
>  	}
> 
> -	ct = kmem_cache_zalloc(nf_conntrack_cachep, gfp);
> +	/*
> +	 * Do not use kmem_cache_zalloc(), as this cache uses
> +	 * SLAB_DESTROY_BY_RCU.
> +	 */
> +	ct = kmem_cache_alloc(nf_conntrack_cachep, gfp);
>  	if (ct == NULL) {
>  		pr_debug("nf_conntrack_alloc: Can't alloc conntrack.\n");
>  		atomic_dec(&net->ct.count);
>  		return ERR_PTR(-ENOMEM);
>  	}
> -
> -	atomic_set(&ct->ct_general.use, 1);
> +	/*
> +	 * Let ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.next
> +	 * and ct->tuplehash[IP_CT_DIR_REPLY].hnnode.next unchanged.
> +	 */
> +	memset(&ct->tuplehash[IP_CT_DIR_MAX], 0,
> +	       sizeof(*ct) - offsetof(struct nf_conn, tuplehash[IP_CT_DIR_MAX]));
>  	ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple = *orig;
> +	ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.pprev = NULL;
>  	ct->tuplehash[IP_CT_DIR_REPLY].tuple = *repl;
> +	ct->tuplehash[IP_CT_DIR_REPLY].hnnode.pprev = NULL;
>  	/* Don't set timer yet: wait for confirmation */
>  	setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);
>  #ifdef CONFIG_NET_NS
>  	ct->ct_net = net;
>  #endif
> 
> +	/*
> +	 * changes to lookup keys must be done before setting refcnt to 1
> +	 */
> +	smp_wmb();
> +	atomic_set(&ct->ct_general.use, 1);
>  	return ct;
>  }
>  EXPORT_SYMBOL_GPL(nf_conntrack_alloc);
> --
> 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: [stable] netfilter -stable 00/08: netfilter -stable fixes

[Greg KH]

On Thu, Jul 23, 2009 at 04:15:24PM +0200, Patrick McHardy wrote:
> Following are couple of netfilter fixes for -stable, fixing

All queued up, thanks.

greg k-h