| Message ID | 20111013020429.3554.78679.stgit@ltc219.sdl.hitachi.co.jp |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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> -- 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
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. -- 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
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 -- 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
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); } }
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(-) -- 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