Skip to contentSkip to conversation

netfilter: bridge: refcount fix

PATCH In review Eric Dumazet 2 participants 4 messages Aug 24, 2009

Subsystems (4)
Networking Network drivers netfilter Ethernet bridge
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 netfilter: bridge: refcount fix

Hi David

I found following by code review only, I am not sure it is critical enough for net-2.6

This is a stable candidate, bug is more than 2 years old.

Thanks

commit f216f082b2b37c4943f1e7c393e2786648d48f6f ([NETFILTER]: bridge netfilter: deal with martians correctly) added a refcount leak on in_dev.

Signed-off-by Eric Dumazet <[email protected]>

---

net/bridge/br_netfilter.c +1 -0
index 4fde742..c62eca3 100644
@@ -386,6 +386,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 				dst_release((struct dst_entry *)rt);
 			}
 free_skb:
+			in_dev_put(in_dev);
 			kfree_skb(skb);
 			return 0;
 		} else {
#1 lore raw
Quoted 19 lines · Eric Dumazet

Hi David

I found following by code review only, I am not sure it is critical enough for net-2.6

This is a stable candidate, bug is more than 2 years old.

Thanks

commit f216f082b2b37c4943f1e7c393e2786648d48f6f ([NETFILTER]: bridge netfilter: deal with martians correctly) added a refcount leak on in_dev.

Signed-off-by Eric Dumazet <[email protected]>

---

net/bridge/br_netfilter.c +0 -0
index 4fde742..c62eca3 100644

net/bridge/br_netfilter.c static int br_nf_pre_routing_finish(struct sk_buff *skb) · line 391

Quoted 6 lines
net/bridge/br_netfilter.c +1 -0
@@ -386,6 +386,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 				dst_release((struct dst_entry *)rt);
 			}
 free_skb:
+			in_dev_put(in_dev);
 			kfree_skb(skb);
 			return 0;

I guess we could simply use __in_dev_get_rcu() here since all netfilter hooks are running under rcu_read_lock() anyways.

author
#2 lore raw
Quoted 13 lines · Patrick McHardy
Quoted 12 lines · Eric Dumazet

Hi David

I found following by code review only, I am not sure it is critical enough for net-2.6

This is a stable candidate, bug is more than 2 years old.

Thanks

commit f216f082b2b37c4943f1e7c393e2786648d48f6f ([NETFILTER]: bridge netfilter: deal with martians correctly) added a refcount leak on in_dev.

Quoted 16 lines

net/bridge/br_netfilter.c static int br_nf_pre_routing_finish(struct sk_buff *skb) · line 391

Quoted 13 lines
Signed-off-by Eric Dumazet <[email protected]>

---

net/bridge/br_netfilter.c +1 -0
index 4fde742..c62eca3 100644
@@ -386,6 +386,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 				dst_release((struct dst_entry *)rt);
 			}
 free_skb:
+			in_dev_put(in_dev);
 			kfree_skb(skb);
 			return 0;

I guess we could simply use __in_dev_get_rcu() here since all netfilter hooks are running under rcu_read_lock() anyways.

Ah very good point, Thanks Patrick.

[PATCH] netfilter: bridge: refcount fix

commit f216f082b2b37c4943f1e7c393e2786648d48f6f ([NETFILTER]: bridge netfilter: deal with martians correctly) added a refcount leak on in_dev.

Instead of using in_dev_get(), we can use __in_dev_get_rcu(), as netfilter hooks are running under rcu_read_lock(), as pointed by Patrick.

Signed-off-by Eric Dumazet <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>

---

net/bridge/br_netfilter.c +1 -1
index 4fde742..907a82e 100644
@@ -359,7 +359,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 				},
 				.proto = 0,
 			};
-			struct in_device *in_dev = in_dev_get(dev);
+			struct in_device *in_dev = __in_dev_get_rcu(dev);
 
 			/* If err equals -EHOSTUNREACH the error is due to a
 			 * martian destination or due to the fact that
#3 lore raw
Quoted 6 lines · Eric Dumazet

[PATCH] netfilter: bridge: refcount fix

commit f216f082b2b37c4943f1e7c393e2786648d48f6f ([NETFILTER]: bridge netfilter: deal with martians correctly) added a refcount leak on in_dev.

Quoted 6 lines

Instead of using in_dev_get(), we can use __in_dev_get_rcu(), as netfilter hooks are running under rcu_read_lock(), as pointed by Patrick.

Signed-off-by Eric Dumazet <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>

I've applied it to nf-next-2.6.git since its not as EARTH SHATTERING as I understood Dave would like it to be for consideration for net-2.6.git :)

Thanks!