* bridge: Fix mglist corruption that leads to memory corruption
@ 2011-02-11 22:36 Herbert Xu
2011-02-11 22:42 ` Herbert Xu
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Herbert Xu @ 2011-02-11 22:36 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: ihands, jbacik
Hi:
This patch fixes a nasty memory corruption issue.
bridge: Fix mglist corruption that leads to memory corruption
The list mp->mglist is used to indicate whether a multicast group
is active on the bridge interface itself as opposed to one of the
constituent interfaces in the bridge.
Unfortunately the operation that adds the mp->mglist node to the
list neglected to check whether it has already been added. This
leads to list corruption in the form of nodes pointing to itself.
Normally this would be quite obvious as it would cause an infinite
loop when walking the list. However, as this list is never actually
walked (which means that we don't really need it, I'll get rid of
it in a subsequent patch), this instead is hidden until we perform
a delete operation on the affected nodes.
As the same node may now be pointed to by more than one node, the
delete operations can then cause modification of freed memory.
This was observed in practice to cause corruption in 512-byte slabs,
most commonly leading to crashes in jbd2.
Thanks to Josef Bacik for pointing me in the right direction.
Reported-by: Ian Page Hands <ihands@redhat.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f701a21..802d3f8 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -719,7 +719,8 @@ static int br_multicast_add_group(struct net_bridge *br,
goto err;
if (!port) {
- hlist_add_head(&mp->mglist, &br->mglist);
+ if (hlist_unhashed(&mp->mglist))
+ hlist_add_head(&mp->mglist, &br->mglist);
mod_timer(&mp->timer, now + br->multicast_membership_interval);
goto out;
}
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-11 22:36 bridge: Fix mglist corruption that leads to memory corruption Herbert Xu
@ 2011-02-11 22:42 ` Herbert Xu
2011-02-12 5:59 ` David Miller
2011-02-11 22:55 ` Herbert Xu
2011-02-12 5:59 ` David Miller
2 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2011-02-11 22:42 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: ihands, jbacik
Hi:
This patch fixes a typo that is not too serious.
bridge: Fix timer typo that may render snooping less effective
In a couple of spots where we are supposed to modify the port
group timer (p->timer) we instead modify the bridge interface
group timer (mp->timer).
The effect of this is mostly harmless. However, it can cause
port subscriptions to be longer than they should be, thus making
snooping less effective.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f701a21..802d3f8 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1177,7 +1178,7 @@ static int br_ip4_multicast_query(struct net_bridge *br,
if (timer_pending(&p->timer) ?
time_after(p->timer.expires, now + max_delay) :
try_to_del_timer_sync(&p->timer) >= 0)
- mod_timer(&mp->timer, now + max_delay);
+ mod_timer(&p->timer, now + max_delay);
}
out:
@@ -1248,7 +1249,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
if (timer_pending(&p->timer) ?
time_after(p->timer.expires, now + max_delay) :
try_to_del_timer_sync(&p->timer) >= 0)
- mod_timer(&mp->timer, now + max_delay);
+ mod_timer(&p->timer, now + max_delay);
}
out:
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-11 22:36 bridge: Fix mglist corruption that leads to memory corruption Herbert Xu
2011-02-11 22:42 ` Herbert Xu
@ 2011-02-11 22:55 ` Herbert Xu
2011-02-12 5:58 ` David Miller
2011-02-12 5:59 ` David Miller
2 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2011-02-11 22:55 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: ihands, jbacik
On Sat, Feb 12, 2011 at 09:36:55AM +1100, Herbert Xu wrote:
>
> Normally this would be quite obvious as it would cause an infinite
> loop when walking the list. However, as this list is never actually
> walked (which means that we don't really need it, I'll get rid of
> it in a subsequent patch), this instead is hidden until we perform
> a delete operation on the affected nodes.
Here is the patch that replaces the mglist hlist with just a bool.
bridge: Replace mp->mglist hlist with a bool
As it turns out we never need to walk through the list of multicast
groups subscribed by the bridge interface itself (the only time we'd
want to do that is when we shut down the bridge, in which case we
simply walk through all multicast groups), we don't really need to
keep an hlist for mp->mglist.
This means that we can replace it with just a single bit to indicate
whether the bridge interface is subscribed to a group.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 6f6d8e1..88e4aa9 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -80,7 +80,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
if (is_multicast_ether_addr(dest)) {
mdst = br_mdb_get(br, skb);
if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
- if ((mdst && !hlist_unhashed(&mdst->mglist)) ||
+ if ((mdst && mdst->mglist) ||
br_multicast_is_router(br))
skb2 = skb;
br_multicast_forward(mdst, skb, skb2);
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index c558274..30e3a08 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -232,8 +232,7 @@ static void br_multicast_group_expired(unsigned long data)
if (!netif_running(br->dev) || timer_pending(&mp->timer))
goto out;
- if (!hlist_unhashed(&mp->mglist))
- hlist_del_init(&mp->mglist);
+ mp->mglist = 0;
if (mp->ports)
goto out;
@@ -276,7 +275,7 @@ static void br_multicast_del_pg(struct net_bridge *br,
del_timer(&p->query_timer);
call_rcu_bh(&p->rcu, br_multicast_free_pg);
- if (!mp->ports && hlist_unhashed(&mp->mglist) &&
+ if (!mp->ports && !mp->mglist &&
netif_running(br->dev))
mod_timer(&mp->timer, jiffies);
@@ -528,7 +527,7 @@ static void br_multicast_group_query_expired(unsigned long data)
struct net_bridge *br = mp->br;
spin_lock(&br->multicast_lock);
- if (!netif_running(br->dev) || hlist_unhashed(&mp->mglist) ||
+ if (!netif_running(br->dev) || !mp->mglist ||
mp->queries_sent >= br->multicast_last_member_count)
goto out;
@@ -719,8 +718,7 @@ static int br_multicast_add_group(struct net_bridge *br,
goto err;
if (!port) {
- if (hlist_unhashed(&mp->mglist))
- hlist_add_head(&mp->mglist, &br->mglist);
+ mp->mglist = 1;
mod_timer(&mp->timer, now + br->multicast_membership_interval);
goto out;
}
@@ -1166,7 +1164,7 @@ static int br_ip4_multicast_query(struct net_bridge *br,
max_delay *= br->multicast_last_member_count;
- if (!hlist_unhashed(&mp->mglist) &&
+ if (mp->mglist &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, now + max_delay) :
try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1237,7 +1235,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
goto out;
max_delay *= br->multicast_last_member_count;
- if (!hlist_unhashed(&mp->mglist) &&
+ if (mp->mglist &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, now + max_delay) :
try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1284,7 +1282,7 @@ static void br_multicast_leave_group(struct net_bridge *br,
br->multicast_last_member_interval;
if (!port) {
- if (!hlist_unhashed(&mp->mglist) &&
+ if (mp->mglist &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, time) :
try_to_del_timer_sync(&mp->timer) >= 0)) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 84aac77..4e1b620 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -84,13 +84,13 @@ struct net_bridge_port_group {
struct net_bridge_mdb_entry
{
struct hlist_node hlist[2];
- struct hlist_node mglist;
struct net_bridge *br;
struct net_bridge_port_group __rcu *ports;
struct rcu_head rcu;
struct timer_list timer;
struct timer_list query_timer;
struct br_ip addr;
+ bool mglist;
u32 queries_sent;
};
@@ -238,7 +238,6 @@ struct net_bridge
spinlock_t multicast_lock;
struct net_bridge_mdb_htable __rcu *mdb;
struct hlist_head router_list;
- struct hlist_head mglist;
struct timer_list multicast_router_timer;
struct timer_list multicast_querier_timer;
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-11 22:55 ` Herbert Xu
@ 2011-02-12 5:58 ` David Miller
2011-02-12 6:05 ` Herbert Xu
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2011-02-12 5:58 UTC (permalink / raw)
To: herbert; +Cc: netdev, ihands, jbacik
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 12 Feb 2011 09:55:59 +1100
> On Sat, Feb 12, 2011 at 09:36:55AM +1100, Herbert Xu wrote:
>>
>> Normally this would be quite obvious as it would cause an infinite
>> loop when walking the list. However, as this list is never actually
>> walked (which means that we don't really need it, I'll get rid of
>> it in a subsequent patch), this instead is hidden until we perform
>> a delete operation on the affected nodes.
>
> Here is the patch that replaces the mglist hlist with just a bool.
>
> bridge: Replace mp->mglist hlist with a bool
>
> As it turns out we never need to walk through the list of multicast
> groups subscribed by the bridge interface itself (the only time we'd
> want to do that is when we shut down the bridge, in which case we
> simply walk through all multicast groups), we don't really need to
> keep an hlist for mp->mglist.
>
> This means that we can replace it with just a single bit to indicate
> whether the bridge interface is subscribed to a group.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Well if it's a bool, please use "true" and "false" instead of "1" and
"0" :-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-11 22:36 bridge: Fix mglist corruption that leads to memory corruption Herbert Xu
2011-02-11 22:42 ` Herbert Xu
2011-02-11 22:55 ` Herbert Xu
@ 2011-02-12 5:59 ` David Miller
2 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2011-02-12 5:59 UTC (permalink / raw)
To: herbert; +Cc: netdev, ihands, jbacik
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 12 Feb 2011 09:36:55 +1100
> This patch fixes a nasty memory corruption issue.
>
> bridge: Fix mglist corruption that leads to memory corruption
Applied.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-11 22:42 ` Herbert Xu
@ 2011-02-12 5:59 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2011-02-12 5:59 UTC (permalink / raw)
To: herbert; +Cc: netdev, ihands, jbacik
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 12 Feb 2011 09:42:07 +1100
> This patch fixes a typo that is not too serious.
>
> bridge: Fix timer typo that may render snooping less effective
Also applied.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-12 5:58 ` David Miller
@ 2011-02-12 6:05 ` Herbert Xu
2011-02-12 9:06 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2011-02-12 6:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, ihands, jbacik
On Fri, Feb 11, 2011 at 09:58:04PM -0800, David Miller wrote:
>
> Well if it's a bool, please use "true" and "false" instead of "1" and
> "0" :-)
Serves me right for trying to be green :)
bridge: Replace mp->mglist hlist with a bool
As it turns out we never need to walk through the list of multicast
groups subscribed by the bridge interface itself (the only time we'd
want to do that is when we shut down the bridge, in which case we
simply walk through all multicast groups), we don't really need to
keep an hlist for mp->mglist.
This means that we can replace it with just a single bit to indicate
whether the bridge interface is subscribed to a group.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 6f6d8e1..88e4aa9 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -80,7 +80,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
if (is_multicast_ether_addr(dest)) {
mdst = br_mdb_get(br, skb);
if (mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) {
- if ((mdst && !hlist_unhashed(&mdst->mglist)) ||
+ if ((mdst && mdst->mglist) ||
br_multicast_is_router(br))
skb2 = skb;
br_multicast_forward(mdst, skb, skb2);
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index c558274..30e3a08 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -232,8 +232,7 @@ static void br_multicast_group_expired(unsigned long data)
if (!netif_running(br->dev) || timer_pending(&mp->timer))
goto out;
- if (!hlist_unhashed(&mp->mglist))
- hlist_del_init(&mp->mglist);
+ mp->mglist = false;
if (mp->ports)
goto out;
@@ -276,7 +275,7 @@ static void br_multicast_del_pg(struct net_bridge *br,
del_timer(&p->query_timer);
call_rcu_bh(&p->rcu, br_multicast_free_pg);
- if (!mp->ports && hlist_unhashed(&mp->mglist) &&
+ if (!mp->ports && !mp->mglist &&
netif_running(br->dev))
mod_timer(&mp->timer, jiffies);
@@ -528,7 +527,7 @@ static void br_multicast_group_query_expired(unsigned long data)
struct net_bridge *br = mp->br;
spin_lock(&br->multicast_lock);
- if (!netif_running(br->dev) || hlist_unhashed(&mp->mglist) ||
+ if (!netif_running(br->dev) || !mp->mglist ||
mp->queries_sent >= br->multicast_last_member_count)
goto out;
@@ -719,8 +718,7 @@ static int br_multicast_add_group(struct net_bridge *br,
goto err;
if (!port) {
- if (hlist_unhashed(&mp->mglist))
- hlist_add_head(&mp->mglist, &br->mglist);
+ mp->mglist = true;
mod_timer(&mp->timer, now + br->multicast_membership_interval);
goto out;
}
@@ -1166,7 +1164,7 @@ static int br_ip4_multicast_query(struct net_bridge *br,
max_delay *= br->multicast_last_member_count;
- if (!hlist_unhashed(&mp->mglist) &&
+ if (mp->mglist &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, now + max_delay) :
try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1237,7 +1235,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
goto out;
max_delay *= br->multicast_last_member_count;
- if (!hlist_unhashed(&mp->mglist) &&
+ if (mp->mglist &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, now + max_delay) :
try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1284,7 +1282,7 @@ static void br_multicast_leave_group(struct net_bridge *br,
br->multicast_last_member_interval;
if (!port) {
- if (!hlist_unhashed(&mp->mglist) &&
+ if (mp->mglist &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, time) :
try_to_del_timer_sync(&mp->timer) >= 0)) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 84aac77..4e1b620 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -84,13 +84,13 @@ struct net_bridge_port_group {
struct net_bridge_mdb_entry
{
struct hlist_node hlist[2];
- struct hlist_node mglist;
struct net_bridge *br;
struct net_bridge_port_group __rcu *ports;
struct rcu_head rcu;
struct timer_list timer;
struct timer_list query_timer;
struct br_ip addr;
+ bool mglist;
u32 queries_sent;
};
@@ -238,7 +238,6 @@ struct net_bridge
spinlock_t multicast_lock;
struct net_bridge_mdb_htable __rcu *mdb;
struct hlist_head router_list;
- struct hlist_head mglist;
struct timer_list multicast_router_timer;
struct timer_list multicast_querier_timer;
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: bridge: Fix mglist corruption that leads to memory corruption
2011-02-12 6:05 ` Herbert Xu
@ 2011-02-12 9:06 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2011-02-12 9:06 UTC (permalink / raw)
To: herbert; +Cc: netdev, ihands, jbacik
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 12 Feb 2011 17:05:38 +1100
> On Fri, Feb 11, 2011 at 09:58:04PM -0800, David Miller wrote:
>>
>> Well if it's a bool, please use "true" and "false" instead of "1" and
>> "0" :-)
>
> Serves me right for trying to be green :)
>
> bridge: Replace mp->mglist hlist with a bool
:-) Applied.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-02-12 9:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-11 22:36 bridge: Fix mglist corruption that leads to memory corruption Herbert Xu
2011-02-11 22:42 ` Herbert Xu
2011-02-12 5:59 ` David Miller
2011-02-11 22:55 ` Herbert Xu
2011-02-12 5:58 ` David Miller
2011-02-12 6:05 ` Herbert Xu
2011-02-12 9:06 ` David Miller
2011-02-12 5:59 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).