* [PATCH net-next] fix NULL pointer dereference in br_handle_frame
@ 2013-09-12 12:16 Hong Zhiguo
2013-09-12 14:11 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Hong Zhiguo @ 2013-09-12 12:16 UTC (permalink / raw)
To: netdev; +Cc: davem, zhiguohong
From: Hong Zhiguo <zhiguohong@tencent.com>
In function netdev_rx_handler_unregister it's said that:
/* a reader seeing a non NULL rx_handler in a rcu_read_lock()
* section has a guarantee to see a non NULL rx_handler_data
* as well.
*/
This is true. But br_port_get_rcu(dev) returns NULL if:
!(dev->priv_flags & IFF_BRIDGE_PORT)
And this happended on my box when br_handle_frame is called
between these 2 lines of del_nbp:
dev->priv_flags &= ~IFF_BRIDGE_PORT;
/* --> br_handle_frame is called at this time */
netdev_upper_dev_unlink(dev, br->dev);
I got below Oops(some lines omitted):
BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
Oops: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff8150901d>] [<ffffffff8150901d>] br_handle_frame+0xed/0x230
RSP: 0018:ffff880030403c10 EFLAGS: 00010286
Stack:
ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
Call Trace:
<IRQ>
[<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
[<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880
Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
net/bridge/br_if.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c41d5fb..bd21159 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -133,6 +133,8 @@ static void del_nbp(struct net_bridge_port *p)
sysfs_remove_link(br->ifobj, p->dev->name);
+ netdev_rx_handler_unregister(dev);
+
dev_set_promiscuity(dev, -1);
spin_lock_bh(&br->lock);
@@ -148,8 +150,6 @@ static void del_nbp(struct net_bridge_port *p)
dev->priv_flags &= ~IFF_BRIDGE_PORT;
- netdev_rx_handler_unregister(dev);
-
netdev_upper_dev_unlink(dev, br->dev);
br_multicast_del_port(p);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 12:16 [PATCH net-next] fix NULL pointer dereference in br_handle_frame Hong Zhiguo
@ 2013-09-12 14:11 ` Eric Dumazet
2013-09-12 14:24 ` Hong zhi guo
2013-09-12 15:57 ` Hong Zhiguo
2013-09-13 3:09 ` [PATCH net-next] bridge: fix NULL pointer deref " Hong Zhiguo
2 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 14:11 UTC (permalink / raw)
To: Hong Zhiguo; +Cc: netdev, davem, zhiguohong
On Thu, 2013-09-12 at 20:16 +0800, Hong Zhiguo wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
>
> In function netdev_rx_handler_unregister it's said that:
>
> /* a reader seeing a non NULL rx_handler in a rcu_read_lock()
> * section has a guarantee to see a non NULL rx_handler_data
> * as well.
> */
>
> This is true. But br_port_get_rcu(dev) returns NULL if:
> !(dev->priv_flags & IFF_BRIDGE_PORT)
>
> And this happended on my box when br_handle_frame is called
> between these 2 lines of del_nbp:
>
> dev->priv_flags &= ~IFF_BRIDGE_PORT;
> /* --> br_handle_frame is called at this time */
> netdev_upper_dev_unlink(dev, br->dev);
>
> I got below Oops(some lines omitted):
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
> IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
> Oops: 0000 [#1] PREEMPT SMP
> RIP: 0010:[<ffffffff8150901d>] [<ffffffff8150901d>] br_handle_frame+0xed/0x230
> RSP: 0018:ffff880030403c10 EFLAGS: 00010286
> Stack:
> ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
> ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
> ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
> Call Trace:
> <IRQ>
> [<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
> [<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
> net/bridge/br_if.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index c41d5fb..bd21159 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -133,6 +133,8 @@ static void del_nbp(struct net_bridge_port *p)
>
> sysfs_remove_link(br->ifobj, p->dev->name);
>
> + netdev_rx_handler_unregister(dev);
> +
> dev_set_promiscuity(dev, -1);
>
> spin_lock_bh(&br->lock);
> @@ -148,8 +150,6 @@ static void del_nbp(struct net_bridge_port *p)
>
> dev->priv_flags &= ~IFF_BRIDGE_PORT;
>
> - netdev_rx_handler_unregister(dev);
> -
> netdev_upper_dev_unlink(dev, br->dev);
>
> br_multicast_del_port(p);
Interesting.
Then br_handle_frame() should not even have to check IFF_BRIDGE_PORT
flag.
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index a2fd37e..45b2568 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
if (!skb)
return RX_HANDLER_CONSUMED;
- p = br_port_get_rcu(skb->dev);
+ p = rcu_dereference(skb->dev->rx_handler_data);
if (unlikely(is_link_local_ether_addr(dest))) {
/*
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 14:11 ` Eric Dumazet
@ 2013-09-12 14:24 ` Hong zhi guo
2013-09-12 14:33 ` Eric Dumazet
0 siblings, 1 reply; 17+ messages in thread
From: Hong zhi guo @ 2013-09-12 14:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller, zhiguohong
You mean IFF_BRIDGE_PORT should be only for admin/control path, but
not for data path?
On Thu, Sep 12, 2013 at 10:11 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-09-12 at 20:16 +0800, Hong Zhiguo wrote:
>> From: Hong Zhiguo <zhiguohong@tencent.com>
>>
>> In function netdev_rx_handler_unregister it's said that:
>>
>> /* a reader seeing a non NULL rx_handler in a rcu_read_lock()
>> * section has a guarantee to see a non NULL rx_handler_data
>> * as well.
>> */
>>
>> This is true. But br_port_get_rcu(dev) returns NULL if:
>> !(dev->priv_flags & IFF_BRIDGE_PORT)
>>
>> And this happended on my box when br_handle_frame is called
>> between these 2 lines of del_nbp:
>>
>> dev->priv_flags &= ~IFF_BRIDGE_PORT;
>> /* --> br_handle_frame is called at this time */
>> netdev_upper_dev_unlink(dev, br->dev);
>>
>> I got below Oops(some lines omitted):
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
>> IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
>> Oops: 0000 [#1] PREEMPT SMP
>> RIP: 0010:[<ffffffff8150901d>] [<ffffffff8150901d>] br_handle_frame+0xed/0x230
>> RSP: 0018:ffff880030403c10 EFLAGS: 00010286
>> Stack:
>> ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
>> ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
>> ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
>> Call Trace:
>> <IRQ>
>> [<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
>> [<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880
>>
>> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
>> ---
>> net/bridge/br_if.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index c41d5fb..bd21159 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -133,6 +133,8 @@ static void del_nbp(struct net_bridge_port *p)
>>
>> sysfs_remove_link(br->ifobj, p->dev->name);
>>
>> + netdev_rx_handler_unregister(dev);
>> +
>> dev_set_promiscuity(dev, -1);
>>
>> spin_lock_bh(&br->lock);
>> @@ -148,8 +150,6 @@ static void del_nbp(struct net_bridge_port *p)
>>
>> dev->priv_flags &= ~IFF_BRIDGE_PORT;
>>
>> - netdev_rx_handler_unregister(dev);
>> -
>> netdev_upper_dev_unlink(dev, br->dev);
>>
>> br_multicast_del_port(p);
>
> Interesting.
>
> Then br_handle_frame() should not even have to check IFF_BRIDGE_PORT
> flag.
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index a2fd37e..45b2568 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> if (!skb)
> return RX_HANDLER_CONSUMED;
>
> - p = br_port_get_rcu(skb->dev);
> + p = rcu_dereference(skb->dev->rx_handler_data);
>
> if (unlikely(is_link_local_ether_addr(dest))) {
> /*
>
>
--
best regards
Hong Zhiguo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 14:24 ` Hong zhi guo
@ 2013-09-12 14:33 ` Eric Dumazet
2013-09-12 14:42 ` Vlad Yasevich
0 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 14:33 UTC (permalink / raw)
To: Hong zhi guo; +Cc: netdev, David Miller, zhiguohong
On Thu, 2013-09-12 at 22:24 +0800, Hong zhi guo wrote:
> You mean IFF_BRIDGE_PORT should be only for admin/control path, but
> not for data path?
By definition, br_handle_frame() should be called only when device is a
bridge port.
After the call to synchronize_net() included in
netdev_rx_handler_unregister(), you have guarantee br_handle_frame()
wont be called.
Therefore, testing IFF_BRIDGE_PORT in br_handle_frame() is redundant.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 14:33 ` Eric Dumazet
@ 2013-09-12 14:42 ` Vlad Yasevich
2013-09-12 15:24 ` Eric Dumazet
0 siblings, 1 reply; 17+ messages in thread
From: Vlad Yasevich @ 2013-09-12 14:42 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Hong zhi guo, netdev, David Miller, zhiguohong
On 09/12/2013 10:33 AM, Eric Dumazet wrote:
> On Thu, 2013-09-12 at 22:24 +0800, Hong zhi guo wrote:
>> You mean IFF_BRIDGE_PORT should be only for admin/control path, but
>> not for data path?
>
> By definition, br_handle_frame() should be called only when device is a
> bridge port.
>
> After the call to synchronize_net() included in
> netdev_rx_handler_unregister(), you have guarantee br_handle_frame()
> wont be called.
>
> Therefore, testing IFF_BRIDGE_PORT in br_handle_frame() is redundant.
Don't all tests for IFF_BRIDGE_PORT on the bridge receive path become
redundant as well?
-vlad
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 14:42 ` Vlad Yasevich
@ 2013-09-12 15:24 ` Eric Dumazet
2013-09-12 15:50 ` Vlad Yasevich
0 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 15:24 UTC (permalink / raw)
To: vyasevic; +Cc: Hong zhi guo, netdev, David Miller, zhiguohong
On Thu, 2013-09-12 at 10:42 -0400, Vlad Yasevich wrote:
> Don't all tests for IFF_BRIDGE_PORT on the bridge receive path become
> redundant as well?
Sure but anyway this part is net-next material
Lets fix the bug first.
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 15:24 ` Eric Dumazet
@ 2013-09-12 15:50 ` Vlad Yasevich
2013-09-12 16:08 ` Eric Dumazet
0 siblings, 1 reply; 17+ messages in thread
From: Vlad Yasevich @ 2013-09-12 15:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Hong zhi guo, netdev, David Miller, zhiguohong
On 09/12/2013 11:24 AM, Eric Dumazet wrote:
> On Thu, 2013-09-12 at 10:42 -0400, Vlad Yasevich wrote:
>
>> Don't all tests for IFF_BRIDGE_PORT on the bridge receive path become
>> redundant as well?
>
> Sure but anyway this part is net-next material
>
> Lets fix the bug first.
Seems like the race is possible in net as well. If we just include the
original patch, then br_add_if() also has to be modified to make sure
the flag is set properly before netdev_rx_handler_register() is called.
If we remove the need to check the flag during bridge input handling,
then br_add_if() change is not necessary.
-vlad
>
> Acked-by: Eric Dumazet <edumazet@google.com>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 12:16 [PATCH net-next] fix NULL pointer dereference in br_handle_frame Hong Zhiguo
2013-09-12 14:11 ` Eric Dumazet
@ 2013-09-12 15:57 ` Hong Zhiguo
2013-09-12 16:06 ` Hong zhi guo
` (2 more replies)
2013-09-13 3:09 ` [PATCH net-next] bridge: fix NULL pointer deref " Hong Zhiguo
2 siblings, 3 replies; 17+ messages in thread
From: Hong Zhiguo @ 2013-09-12 15:57 UTC (permalink / raw)
To: vyasevic; +Cc: eric.dumazet, davem, netdev, Hong Zhiguo
not check IFF_BRIDGE_PORT within br_handle_frame
Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
net/bridge/br_input.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index a2fd37e..da4714a 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
int br_handle_frame_finish(struct sk_buff *skb)
{
const unsigned char *dest = eth_hdr(skb)->h_dest;
- struct net_bridge_port *p = br_port_get_rcu(skb->dev);
+ enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
struct net_bridge *br;
struct net_bridge_fdb_entry *dst;
struct net_bridge_mdb_entry *mdst;
@@ -143,7 +143,7 @@ drop:
/* note: already called with rcu_read_lock */
static int br_handle_local_finish(struct sk_buff *skb)
{
- struct net_bridge_port *p = br_port_get_rcu(skb->dev);
+ struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
u16 vid = 0;
br_vlan_get_tag(skb, &vid);
@@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
if (!skb)
return RX_HANDLER_CONSUMED;
- p = br_port_get_rcu(skb->dev);
+ p = rcu_dereference(skb->dev->rx_handler_data);
if (unlikely(is_link_local_ether_addr(dest))) {
/*
--
1.7.0.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 15:57 ` Hong Zhiguo
@ 2013-09-12 16:06 ` Hong zhi guo
2013-09-12 16:11 ` Eric Dumazet
2013-09-12 16:12 ` Eric Dumazet
2013-09-12 19:18 ` David Miller
2 siblings, 1 reply; 17+ messages in thread
From: Hong zhi guo @ 2013-09-12 16:06 UTC (permalink / raw)
To: vyasevic; +Cc: Eric Dumazet, David Miller, netdev, Hong Zhiguo
You're right, Vlad.
One thing is missing in Eric's fix, NULL dereference is still possible
in br_handle_local_finish. Above is the new version of fix.
On Thu, Sep 12, 2013 at 11:57 PM, Hong Zhiguo <honkiko@gmail.com> wrote:
> not check IFF_BRIDGE_PORT within br_handle_frame
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
> net/bridge/br_input.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index a2fd37e..da4714a 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
> int br_handle_frame_finish(struct sk_buff *skb)
> {
> const unsigned char *dest = eth_hdr(skb)->h_dest;
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> struct net_bridge *br;
> struct net_bridge_fdb_entry *dst;
> struct net_bridge_mdb_entry *mdst;
> @@ -143,7 +143,7 @@ drop:
> /* note: already called with rcu_read_lock */
> static int br_handle_local_finish(struct sk_buff *skb)
> {
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> u16 vid = 0;
>
> br_vlan_get_tag(skb, &vid);
> @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> if (!skb)
> return RX_HANDLER_CONSUMED;
>
> - p = br_port_get_rcu(skb->dev);
> + p = rcu_dereference(skb->dev->rx_handler_data);
>
> if (unlikely(is_link_local_ether_addr(dest))) {
> /*
> --
> 1.7.0.4
>
--
best regards
Hong Zhiguo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 15:50 ` Vlad Yasevich
@ 2013-09-12 16:08 ` Eric Dumazet
0 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 16:08 UTC (permalink / raw)
To: vyasevic; +Cc: Hong zhi guo, netdev, David Miller, zhiguohong
On Thu, 2013-09-12 at 11:50 -0400, Vlad Yasevich wrote:
> On 09/12/2013 11:24 AM, Eric Dumazet wrote:
> > On Thu, 2013-09-12 at 10:42 -0400, Vlad Yasevich wrote:
> >
> >> Don't all tests for IFF_BRIDGE_PORT on the bridge receive path become
> >> redundant as well?
> >
> > Sure but anyway this part is net-next material
> >
> > Lets fix the bug first.
>
> Seems like the race is possible in net as well. If we just include the
> original patch, then br_add_if() also has to be modified to make sure
> the flag is set properly before netdev_rx_handler_register() is called.
>
> If we remove the need to check the flag during bridge input handling,
> then br_add_if() change is not necessary.
Yep, really testing IFF_BRIDGE_PORT in fast path is currently racy.
rcu protection should be enough.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 16:06 ` Hong zhi guo
@ 2013-09-12 16:11 ` Eric Dumazet
2013-09-12 16:18 ` Hong zhi guo
0 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 16:11 UTC (permalink / raw)
To: Hong zhi guo; +Cc: vyasevic, David Miller, netdev, Hong Zhiguo
On Fri, 2013-09-13 at 00:06 +0800, Hong zhi guo wrote:
> You're right, Vlad.
> One thing is missing in Eric's fix, NULL dereference is still possible
> in br_handle_local_finish. Above is the new version of fix.
Hey, it was not a 'fix', but a comment on your patch and bridge
defensive programming.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 15:57 ` Hong Zhiguo
2013-09-12 16:06 ` Hong zhi guo
@ 2013-09-12 16:12 ` Eric Dumazet
2013-09-12 19:18 ` David Miller
2 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 16:12 UTC (permalink / raw)
To: Hong Zhiguo; +Cc: vyasevic, davem, netdev, Hong Zhiguo
On Thu, 2013-09-12 at 23:57 +0800, Hong Zhiguo wrote:
> not check IFF_BRIDGE_PORT within br_handle_frame
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
No need to hurry. Take the time to write an extensive changelog, and
test the patch !
Thanks
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 16:11 ` Eric Dumazet
@ 2013-09-12 16:18 ` Hong zhi guo
2013-09-12 16:19 ` Eric Dumazet
0 siblings, 1 reply; 17+ messages in thread
From: Hong zhi guo @ 2013-09-12 16:18 UTC (permalink / raw)
To: Eric Dumazet; +Cc: vyasevic, David Miller, netdev, Hong Zhiguo
sorry, Eric, maybe I'm using wrong words. Thanks for your review and help.
So you both prefer not testing IFF_BRIDGE_PORT. Let's take the new fix.
On Fri, Sep 13, 2013 at 12:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-09-13 at 00:06 +0800, Hong zhi guo wrote:
>> You're right, Vlad.
>> One thing is missing in Eric's fix, NULL dereference is still possible
>> in br_handle_local_finish. Above is the new version of fix.
>
> Hey, it was not a 'fix', but a comment on your patch and bridge
> defensive programming.
>
>
>
--
best regards
Hong Zhiguo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 16:18 ` Hong zhi guo
@ 2013-09-12 16:19 ` Eric Dumazet
0 siblings, 0 replies; 17+ messages in thread
From: Eric Dumazet @ 2013-09-12 16:19 UTC (permalink / raw)
To: Hong zhi guo; +Cc: vyasevic, David Miller, netdev, Hong Zhiguo
On Fri, 2013-09-13 at 00:18 +0800, Hong zhi guo wrote:
> sorry, Eric, maybe I'm using wrong words. Thanks for your review and help.
> So you both prefer not testing IFF_BRIDGE_PORT. Let's take the new fix.
Please do not top post on netdev.
Thanks
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] fix NULL pointer dereference in br_handle_frame
2013-09-12 15:57 ` Hong Zhiguo
2013-09-12 16:06 ` Hong zhi guo
2013-09-12 16:12 ` Eric Dumazet
@ 2013-09-12 19:18 ` David Miller
2 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2013-09-12 19:18 UTC (permalink / raw)
To: honkiko; +Cc: vyasevic, eric.dumazet, netdev, zhiguohong
From: Hong Zhiguo <honkiko@gmail.com>
Date: Thu, 12 Sep 2013 23:57:08 +0800
> @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
> int br_handle_frame_finish(struct sk_buff *skb)
> {
> const unsigned char *dest = eth_hdr(skb)->h_dest;
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + enet_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
You did not even compile test this patch.
This is a good way to make people not want to review your work at all,
because if you don't care enough to even compile test your patch why
should other people care enough to look at it?
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next] bridge: fix NULL pointer deref in br_handle_frame
2013-09-12 12:16 [PATCH net-next] fix NULL pointer dereference in br_handle_frame Hong Zhiguo
2013-09-12 14:11 ` Eric Dumazet
2013-09-12 15:57 ` Hong Zhiguo
@ 2013-09-13 3:09 ` Hong Zhiguo
2013-09-13 16:21 ` Stephen Hemminger
2 siblings, 1 reply; 17+ messages in thread
From: Hong Zhiguo @ 2013-09-13 3:09 UTC (permalink / raw)
To: netdev; +Cc: davem, zhiguohong, eric.dumazet, vyasevic
From: Hong Zhiguo <zhiguohong@tencent.com>
I got an Oops on my box when br_handle_frame is called between these
2 lines of del_nbp:
dev->priv_flags &= ~IFF_BRIDGE_PORT;
/* --> br_handle_frame is called at this time */
netdev_rx_handler_unregister(dev);
In br_handle_frame the return of br_port_get_rcu(dev) is dereferenced
without check but br_port_get_rcu(dev) returns NULL if:
!(dev->priv_flags & IFF_BRIDGE_PORT)
In my first fix I moved netdev_rx_handler_unregister up. Eric Dumazet
pointed out the testing of IFF_BRIDGE_PORT is not necessary here since
we're in rcu_read_lock and we have synchronize_net() in
netdev_rx_handler_unregister. This fix removed the testing of
IFF_BRIDGE_PORT.
I tested the fix on my box with script doing "brctl addif" and "brctl
delif" repeatedly while a lot of broadcast frame present on the LAN.
I added msleep in del_nbp between setting of priv_flags and unregister
so it's easy to reproduce the oops without the fix.
I'll send another patch to net-next to take care of br_netfilter and
ebtable if necessary(seems there's NULL check following but I'll
have a look).
The Oops(some lines omitted):
BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
Oops: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff8150901d>] [<ffffffff8150901d>] br_handle_frame+0xed/0x230
RSP: 0018:ffff880030403c10 EFLAGS: 00010286
Stack:
ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
Call Trace:
<IRQ>
[<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
[<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880
Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
---
net/bridge/br_input.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index a2fd37e..2244049 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
int br_handle_frame_finish(struct sk_buff *skb)
{
const unsigned char *dest = eth_hdr(skb)->h_dest;
- struct net_bridge_port *p = br_port_get_rcu(skb->dev);
+ struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
struct net_bridge *br;
struct net_bridge_fdb_entry *dst;
struct net_bridge_mdb_entry *mdst;
@@ -143,7 +143,7 @@ drop:
/* note: already called with rcu_read_lock */
static int br_handle_local_finish(struct sk_buff *skb)
{
- struct net_bridge_port *p = br_port_get_rcu(skb->dev);
+ struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
u16 vid = 0;
br_vlan_get_tag(skb, &vid);
@@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
if (!skb)
return RX_HANDLER_CONSUMED;
- p = br_port_get_rcu(skb->dev);
+ p = rcu_dereference(skb->dev->rx_handler_data);
if (unlikely(is_link_local_ether_addr(dest))) {
/*
--
1.8.1.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next] bridge: fix NULL pointer deref in br_handle_frame
2013-09-13 3:09 ` [PATCH net-next] bridge: fix NULL pointer deref " Hong Zhiguo
@ 2013-09-13 16:21 ` Stephen Hemminger
0 siblings, 0 replies; 17+ messages in thread
From: Stephen Hemminger @ 2013-09-13 16:21 UTC (permalink / raw)
To: Hong Zhiguo; +Cc: netdev, davem, zhiguohong, eric.dumazet, vyasevic
On Fri, 13 Sep 2013 11:09:47 +0800
Hong Zhiguo <honkiko@gmail.com> wrote:
> From: Hong Zhiguo <zhiguohong@tencent.com>
>
> I got an Oops on my box when br_handle_frame is called between these
> 2 lines of del_nbp:
> dev->priv_flags &= ~IFF_BRIDGE_PORT;
> /* --> br_handle_frame is called at this time */
> netdev_rx_handler_unregister(dev);
>
> In br_handle_frame the return of br_port_get_rcu(dev) is dereferenced
> without check but br_port_get_rcu(dev) returns NULL if:
> !(dev->priv_flags & IFF_BRIDGE_PORT)
>
> In my first fix I moved netdev_rx_handler_unregister up. Eric Dumazet
> pointed out the testing of IFF_BRIDGE_PORT is not necessary here since
> we're in rcu_read_lock and we have synchronize_net() in
> netdev_rx_handler_unregister. This fix removed the testing of
> IFF_BRIDGE_PORT.
>
> I tested the fix on my box with script doing "brctl addif" and "brctl
> delif" repeatedly while a lot of broadcast frame present on the LAN.
> I added msleep in del_nbp between setting of priv_flags and unregister
> so it's easy to reproduce the oops without the fix.
>
> I'll send another patch to net-next to take care of br_netfilter and
> ebtable if necessary(seems there's NULL check following but I'll
> have a look).
>
> The Oops(some lines omitted):
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000021
> IP: [<ffffffff8150901d>] br_handle_frame+0xed/0x230
> Oops: 0000 [#1] PREEMPT SMP
> RIP: 0010:[<ffffffff8150901d>] [<ffffffff8150901d>] br_handle_frame+0xed/0x230
> RSP: 0018:ffff880030403c10 EFLAGS: 00010286
> Stack:
> ffff88002c945700 ffffffff81508f30 0000000000000000 ffff88002d41e000
> ffff880030403c98 ffffffff81477acb ffffffff81477821 ffff880030403c68
> ffffffff81090e10 00ff88002d545c80 ffff88002c945700 ffffffff81aa50c0
> Call Trace:
> <IRQ>
> [<ffffffff81508f30>] ? br_handle_frame_finish+0x300/0x300
> [<ffffffff81477acb>] __netif_receive_skb_core+0x39b/0x880
>
> Signed-off-by: Hong Zhiguo <zhiguohong@tencent.com>
> ---
> net/bridge/br_input.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index a2fd37e..2244049 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -60,7 +60,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
> int br_handle_frame_finish(struct sk_buff *skb)
> {
> const unsigned char *dest = eth_hdr(skb)->h_dest;
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> struct net_bridge *br;
> struct net_bridge_fdb_entry *dst;
> struct net_bridge_mdb_entry *mdst;
> @@ -143,7 +143,7 @@ drop:
> /* note: already called with rcu_read_lock */
> static int br_handle_local_finish(struct sk_buff *skb)
> {
> - struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> + struct net_bridge_port *p = rcu_dereference(skb->dev->rx_handler_data);
> u16 vid = 0;
>
> br_vlan_get_tag(skb, &vid);
> @@ -173,7 +173,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> if (!skb)
> return RX_HANDLER_CONSUMED;
>
> - p = br_port_get_rcu(skb->dev);
> + p = rcu_dereference(skb->dev->rx_handler_data);
>
> if (unlikely(is_link_local_ether_addr(dest))) {
> /*
There are more uses of br_port_get_rcu that have the same problem.
For example receiving an STP packet in that window.
I bet if you look at all the callers of br_port_get_rcu() you will
see the same issue, therefore either the check should be removed from br_port_get_rcu.
A little bit of history, the bridge code orginally did not use RCU,
and there was a flag on the device. Then RCU was added, then the receive
handler stuff was added.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-09-13 16:21 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-12 12:16 [PATCH net-next] fix NULL pointer dereference in br_handle_frame Hong Zhiguo
2013-09-12 14:11 ` Eric Dumazet
2013-09-12 14:24 ` Hong zhi guo
2013-09-12 14:33 ` Eric Dumazet
2013-09-12 14:42 ` Vlad Yasevich
2013-09-12 15:24 ` Eric Dumazet
2013-09-12 15:50 ` Vlad Yasevich
2013-09-12 16:08 ` Eric Dumazet
2013-09-12 15:57 ` Hong Zhiguo
2013-09-12 16:06 ` Hong zhi guo
2013-09-12 16:11 ` Eric Dumazet
2013-09-12 16:18 ` Hong zhi guo
2013-09-12 16:19 ` Eric Dumazet
2013-09-12 16:12 ` Eric Dumazet
2013-09-12 19:18 ` David Miller
2013-09-13 3:09 ` [PATCH net-next] bridge: fix NULL pointer deref " Hong Zhiguo
2013-09-13 16:21 ` Stephen Hemminger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.