New: net_device refcnt bug when NFQUEUEing bridged packets

Bugme-new Andrew Morton 3 participants 5 messages Jan 15, 2008 – Jan 20, 2008 1 message not in the archive

Subsystems (4)

Networking Network drivers netfilter Ethernet bridge

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author

2008-01-15 23:57 UTC #1 lore raw

Quoted 40 lines · On Tue, 15 Jan 2008 15:28:31 -0800 (PST) [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=9758
           Summary: net_device refcnt bug when NFQUEUEing bridged packets
           Product: Networking
           Version: 2.5
     KernelVersion: 2.6.24-rc7
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Netfilter/Iptables
        AssignedTo: [email protected]
        ReportedBy: [email protected]
The bug is probably around since the combination bridge+NFQUEUE is possible, and does not depend on distro or environment:
Packets that are to be sent out over a bridge device are skb_clone()d in br_loop() before traversing the appropriate (FORWARD/OUTPUT) NF chain. The copies made by skb_clone() share their nf_bridge metadata with the original, which is no problem usually.
If however one or more packets of a br_loop() run end up in a NFQUEUE, their shared nf_bridge metadata causes trouble when they are about to be
reinjected: nf_reinject() decrements the net_device refcounts that were previously upped when queueing the packet in __nf_queue(), but as skb->nf_bridge->physoutdev points to the same device for all these packets, most (if not all) of them will affect the wrong refcnt.
(I originally encountered the bug on a Xen host because the hypervisor refused to shutdown a virtual device with non-zero refcount... but it is perfectly reproducible with a standard kernel, too, although it was a bit more tedious to create a test scenario, involving a couple of UMLs.)
I'd suggest to make a real copy of the nf_bridge member in br_loop() if CONFIG_BRIDGE_NETFILTER is defined, remedying the entanglement. I'd go ahead and create a patch, but I'm unsure as to where that logic should be implemented.

Patrick McHardy

2008-01-16 04:54 UTC #2 lore raw

Quoted 24 lines · Andrew Morton

Quoted 21 lines · On Tue, 15 Jan 2008 15:28:31 -0800 (PST) [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=9758
The bug is probably around since the combination bridge+NFQUEUE is possible, and does not depend on distro or environment:
Packets that are to be sent out over a bridge device are skb_clone()d in br_loop() before traversing the appropriate (FORWARD/OUTPUT) NF chain. The copies made by skb_clone() share their nf_bridge metadata with the original, which is no problem usually.
If however one or more packets of a br_loop() run end up in a NFQUEUE, their shared nf_bridge metadata causes trouble when they are about to be
reinjected: nf_reinject() decrements the net_device refcounts that were previously upped when queueing the packet in __nf_queue(), but as skb->nf_bridge->physoutdev points to the same device for all these packets, most (if not all) of them will affect the wrong refcnt.
(I originally encountered the bug on a Xen host because the hypervisor refused to shutdown a virtual device with non-zero refcount... but it is perfectly reproducible with a standard kernel, too, although it was a bit more tedious to create a test scenario, involving a couple of UMLs.)

Quoted 4 lines

Quoted 4 lines
I'd suggest to make a real copy of the nf_bridge member in br_loop() if CONFIG_BRIDGE_NETFILTER is defined, remedying the entanglement. I'd go ahead and create a patch, but I'm unsure as to where that logic should be implemented.

Very nice catch, that explains quite a few bug reports about refcnt leaks. Your patch looks correct and performs the copying in the logically correct place, it would be nicer to keep this crap limited to bridge netfilter however.

What should work is to perform the copying in br_netfilter.c at the spots where phsyoutdev is assigned. As an optimization we should be able to avoid the copying in most cases by checking that the bridge info has a refcount above 1.

Could you test whether this patch also fixes the problem?

net/bridge/br_netfilter.c +26 -0

index 0e884fe..9759bd7 100644
@@ -142,6 +142,22 @@ static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
 	return skb->nf_bridge;
 }
 
+static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
+{
+	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+
+	if (atomic_read(&nf_bridge->use) > 1) {
+		struct nf_bridge_info *tmp = nf_bridge_alloc(skb);
+
+		if (tmp) {
+			memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info));
+			nf_bridge_put(nf_bridge);
+		}
+		nf_bridge = tmp;
+	}
+	return nf_bridge;
+}
+
 static inline void nf_bridge_push_encap_header(struct sk_buff *skb)
 {
 	unsigned int len = nf_bridge_encap_header_len(skb);

@@ -637,6 +653,11 @@ static unsigned int br_nf_forward_ip(unsigned int hook, struct sk_buff *skb,
 	if (!skb->nf_bridge)
 		return NF_ACCEPT;
 
+	/* Need exclusive nf_bridge_info since we might have multiple
+	 * different physoutdevs. */
+	if (!nf_bridge_unshare(skb))
+		return NF_DROP;
+
 	parent = bridge_parent(out);
 	if (!parent)
 		return NF_DROP;

@@ -718,6 +739,11 @@ static unsigned int br_nf_local_out(unsigned int hook, struct sk_buff *skb,
 	if (!skb->nf_bridge)
 		return NF_ACCEPT;
 
+	/* Need exclusive nf_bridge_info since we might have multiple
+	 * different physoutdevs. */
+	if (!nf_bridge_unshare(skb))
+		return NF_DROP;
+
 	nf_bridge = skb->nf_bridge;
 	if (!(nf_bridge->mask & BRNF_BRIDGED_DNAT))
 		return NF_ACCEPT;

Patrick McHardy

2008-01-16 04:59 UTC #3 lore raw

Quoted 11 lines · Patrick McHardy

Very nice catch, that explains quite a few bug reports about refcnt leaks. Your patch looks correct and performs the copying in the logically correct place, it would be nicer to keep this crap limited to bridge netfilter however.
What should work is to perform the copying in br_netfilter.c at the spots where phsyoutdev is assigned. As an optimization we should be able to avoid the copying in most cases by checking that the bridge info has a refcount above 1.
Could you test whether this patch also fixes the problem?

That patch had a bug, we need to set the refcount of the new bridge info to 1 after performing the copy.

net/bridge/br_netfilter.c +27 -0

index 0e884fe..141f069 100644
@@ -142,6 +142,23 @@ static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
 	return skb->nf_bridge;
 }
 
+static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
+{
+	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+
+	if (atomic_read(&nf_bridge->use) > 1) {
+		struct nf_bridge_info *tmp = nf_bridge_alloc(skb);
+
+		if (tmp) {
+			memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info));
+			atomic_set(&tmp->use, 1);
+			nf_bridge_put(nf_bridge);
+		}
+		nf_bridge = tmp;
+	}
+	return nf_bridge;
+}
+
 static inline void nf_bridge_push_encap_header(struct sk_buff *skb)
 {
 	unsigned int len = nf_bridge_encap_header_len(skb);

@@ -637,6 +654,11 @@ static unsigned int br_nf_forward_ip(unsigned int hook, struct sk_buff *skb,
 	if (!skb->nf_bridge)
 		return NF_ACCEPT;
 
+	/* Need exclusive nf_bridge_info since we might have multiple
+	 * different physoutdevs. */
+	if (!nf_bridge_unshare(skb))
+		return NF_DROP;
+
 	parent = bridge_parent(out);
 	if (!parent)
 		return NF_DROP;

@@ -718,6 +740,11 @@ static unsigned int br_nf_local_out(unsigned int hook, struct sk_buff *skb,
 	if (!skb->nf_bridge)
 		return NF_ACCEPT;
 
+	/* Need exclusive nf_bridge_info since we might have multiple
+	 * different physoutdevs. */
+	if (!nf_bridge_unshare(skb))
+		return NF_DROP;
+
 	nf_bridge = skb->nf_bridge;
 	if (!(nf_bridge->mask & BRNF_BRIDGED_DNAT))
 		return NF_ACCEPT;

Stephen Hemminger

2008-01-16 18:56 UTC #4 lore raw

Quoted 11 lines · On Wed, 16 Jan 2008 05:59:21 +0100 Patrick McHardy <[email protected]> wrote:

Quoted 10 lines · Patrick McHardy
Very nice catch, that explains quite a few bug reports about refcnt leaks. Your patch looks correct and performs the copying in the logically correct place, it would be nicer to keep this crap limited to bridge netfilter however.
What should work is to perform the copying in br_netfilter.c at the spots where phsyoutdev is assigned. As an optimization we should be able to avoid the copying in most cases by checking that the bridge info has a refcount above 1.

Quoted 6 lines

Quoted 1 line
Could you test whether this patch also fixes the problem?
That patch had a bug, we need to set the refcount of the new bridge info to 1 after performing the copy.

This looks good, but you could use a structure assignment rather memcpy (just a personal style preference because assignment is typed and memcpy is not).

Patrick McHardy

2008-01-20 13:59 UTC #5 lore raw

Quoted 3 lines · Stephen Hemminger

This looks good, but you could use a structure assignment rather memcpy (just a personal style preference because assignment is typed and memcpy is not).

That doesn't work in this case since nf_bridge_info contains

        unsigned long data[32 / sizeof(unsigned long)];

and I don't want to assign each array element seperately.