* [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame
@ 2011-10-13 2:04 Mitsuo Hayasaka
2011-10-19 4:03 ` David Miller
2011-10-19 4:05 ` Eric Dumazet
0 siblings, 2 replies; 5+ messages in thread
From: Mitsuo Hayasaka @ 2011-10-13 2:04 UTC (permalink / raw)
To: Jay Vosburgh, Andy Gospodarek
Cc: netdev, linux-kernel, yrl.pp-manager.tt, Mitsuo Hayasaka,
Jay Vosburgh, Andy Gospodarek, Eric Dumazet, WANG Cong
The bond->recv_probe is called in bond_handle_frame() when
a packet is received, but bond_close() sets it to NULL. So,
a panic occurs when both functions work in parallel.
Why this happen:
After null pointer check of bond->recv_probe, an sk_buff is
duplicated and bond->recv_probe is called in bond_handle_frame.
So, a panic occurs when bond_close() is called between the
check and call of bond->recv_probe.
Patch:
This patch uses a local function pointer of bond->recv_probe
in bond_handle_frame(). So, it can avoid the null pointer
dereference.
Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: WANG Cong <xiyou.wangcong@gmail.com>
---
drivers/net/bonding/bond_main.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6d79b78..de3d351 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1435,6 +1435,8 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
struct sk_buff *skb = *pskb;
struct slave *slave;
struct bonding *bond;
+ void (*recv_probe)(struct sk_buff *, struct bonding *,
+ struct slave *);
skb = skb_share_check(skb, GFP_ATOMIC);
if (unlikely(!skb))
@@ -1448,11 +1450,12 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
if (bond->params.arp_interval)
slave->dev->last_rx = jiffies;
- if (bond->recv_probe) {
+ recv_probe = ACCESS_ONCE(bond->recv_probe);
+ if (recv_probe) {
struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
if (likely(nskb)) {
- bond->recv_probe(nskb, bond, slave);
+ recv_probe(nskb, bond, slave);
dev_kfree_skb(nskb);
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame
2011-10-13 2:04 [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame Mitsuo Hayasaka
@ 2011-10-19 4:03 ` David Miller
2011-10-19 17:59 ` Jay Vosburgh
2011-10-19 4:05 ` Eric Dumazet
1 sibling, 1 reply; 5+ messages in thread
From: David Miller @ 2011-10-19 4:03 UTC (permalink / raw)
To: mitsuo.hayasaka.hu
Cc: fubar, andy, netdev, linux-kernel, yrl.pp-manager.tt,
eric.dumazet, xiyou.wangcong
From: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Date: Thu, 13 Oct 2011 11:04:29 +0900
> The bond->recv_probe is called in bond_handle_frame() when
> a packet is received, but bond_close() sets it to NULL. So,
> a panic occurs when both functions work in parallel.
>
> Why this happen:
> After null pointer check of bond->recv_probe, an sk_buff is
> duplicated and bond->recv_probe is called in bond_handle_frame.
> So, a panic occurs when bond_close() is called between the
> check and call of bond->recv_probe.
>
> Patch:
> This patch uses a local function pointer of bond->recv_probe
> in bond_handle_frame(). So, it can avoid the null pointer
> dereference.
>
>
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Jay Vosburgh <fubar@us.ibm.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
Bonding folks please review this, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame
2011-10-13 2:04 [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame Mitsuo Hayasaka
2011-10-19 4:03 ` David Miller
@ 2011-10-19 4:05 ` Eric Dumazet
2011-10-19 4:14 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2011-10-19 4:05 UTC (permalink / raw)
To: Mitsuo Hayasaka
Cc: Jay Vosburgh, Andy Gospodarek, netdev, linux-kernel,
yrl.pp-manager.tt, WANG Cong
Le jeudi 13 octobre 2011 à 11:04 +0900, Mitsuo Hayasaka a écrit :
> The bond->recv_probe is called in bond_handle_frame() when
> a packet is received, but bond_close() sets it to NULL. So,
> a panic occurs when both functions work in parallel.
>
> Why this happen:
> After null pointer check of bond->recv_probe, an sk_buff is
> duplicated and bond->recv_probe is called in bond_handle_frame.
> So, a panic occurs when bond_close() is called between the
> check and call of bond->recv_probe.
>
> Patch:
> This patch uses a local function pointer of bond->recv_probe
> in bond_handle_frame(). So, it can avoid the null pointer
> dereference.
>
>
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Jay Vosburgh <fubar@us.ibm.com>
> Cc: Andy Gospodarek <andy@greyhouse.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
> ---
>
> drivers/net/bonding/bond_main.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6d79b78..de3d351 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1435,6 +1435,8 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *skb = *pskb;
> struct slave *slave;
> struct bonding *bond;
> + void (*recv_probe)(struct sk_buff *, struct bonding *,
> + struct slave *);
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (unlikely(!skb))
> @@ -1448,11 +1450,12 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> if (bond->params.arp_interval)
> slave->dev->last_rx = jiffies;
>
> - if (bond->recv_probe) {
> + recv_probe = ACCESS_ONCE(bond->recv_probe);
> + if (recv_probe) {
> struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>
> if (likely(nskb)) {
> - bond->recv_probe(nskb, bond, slave);
> + recv_probe(nskb, bond, slave);
> dev_kfree_skb(nskb);
> }
> }
>
Sorry, I forgot to add my official ack. Even if not a perfect patch, its
a step into right direction.
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame
2011-10-19 4:05 ` Eric Dumazet
@ 2011-10-19 4:14 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2011-10-19 4:14 UTC (permalink / raw)
To: eric.dumazet
Cc: mitsuo.hayasaka.hu, fubar, andy, netdev, linux-kernel,
yrl.pp-manager.tt, xiyou.wangcong
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 19 Oct 2011 06:05:27 +0200
> Le jeudi 13 octobre 2011 à 11:04 +0900, Mitsuo Hayasaka a écrit :
>> The bond->recv_probe is called in bond_handle_frame() when
>> a packet is received, but bond_close() sets it to NULL. So,
>> a panic occurs when both functions work in parallel.
>>
>> Why this happen:
>> After null pointer check of bond->recv_probe, an sk_buff is
>> duplicated and bond->recv_probe is called in bond_handle_frame.
>> So, a panic occurs when bond_close() is called between the
>> check and call of bond->recv_probe.
>>
>> Patch:
>> This patch uses a local function pointer of bond->recv_probe
>> in bond_handle_frame(). So, it can avoid the null pointer
>> dereference.
>>
>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
...
> Sorry, I forgot to add my official ack. Even if not a perfect patch, its
> a step into right direction.
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Thanks for reviewing Eric, applied.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame
2011-10-19 4:03 ` David Miller
@ 2011-10-19 17:59 ` Jay Vosburgh
0 siblings, 0 replies; 5+ messages in thread
From: Jay Vosburgh @ 2011-10-19 17:59 UTC (permalink / raw)
To: David Miller
Cc: mitsuo.hayasaka.hu, andy, netdev, linux-kernel, yrl.pp-manager.tt,
eric.dumazet, xiyou.wangcong
David Miller <davem@davemloft.net> wrote:
>From: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>Date: Thu, 13 Oct 2011 11:04:29 +0900
>
>> The bond->recv_probe is called in bond_handle_frame() when
>> a packet is received, but bond_close() sets it to NULL. So,
>> a panic occurs when both functions work in parallel.
>>
>> Why this happen:
>> After null pointer check of bond->recv_probe, an sk_buff is
>> duplicated and bond->recv_probe is called in bond_handle_frame.
>> So, a panic occurs when bond_close() is called between the
>> check and call of bond->recv_probe.
>>
>> Patch:
>> This patch uses a local function pointer of bond->recv_probe
>> in bond_handle_frame(). So, it can avoid the null pointer
>> dereference.
>>
>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Andy Gospodarek <andy@greyhouse.net>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Cc: WANG Cong <xiyou.wangcong@gmail.com>
>
>Bonding folks please review this, thanks.
>
Looks reasonable. Even if by some quirk of timing the
recv_probe function ends up being entered after bond_close has
completed, it doesn't look like there is a risk of those functions
misbehaving (because bond_close doesn't deallocate the data structures).
-J
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-10-19 17:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-13 2:04 [PATCH net -v2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame Mitsuo Hayasaka
2011-10-19 4:03 ` David Miller
2011-10-19 17:59 ` Jay Vosburgh
2011-10-19 4:05 ` Eric Dumazet
2011-10-19 4:14 ` 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).