[PATCH net-next] 802: fix a possible race condition

[PATCH net-next] 802: fix a possible race condition

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

garp_pdu_queue() should ways be called with this spin lock.
garp_uninit_applicant() only holds rtnl lock which is not
enough here.

Found by code inspection.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ward <david.ward@ll.mit.edu>
Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/802/garp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8456f5d..5d9630a 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -609,8 +609,12 @@ void garp_uninit_applicant(struct net_device *dev, struct garp_application *appl
 	/* Delete timer and generate a final TRANSMIT_PDU event to flush out
 	 * all pending messages before the applicant is gone. */
 	del_timer_sync(&app->join_timer);
+
+	spin_lock_bh(&app->lock);
 	garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
 	garp_pdu_queue(app);
+	spin_unlock_bh(&app->lock);
+
 	garp_queue_xmit(app);
 
 	dev_mc_del(dev, appl->proto.group_address);
-- 
1.7.7.6

Re: [PATCH net-next] 802: fix a possible race condition

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Sat, 23 Mar 2013 13:14:08 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> garp_pdu_queue() should ways be called with this spin lock.
> garp_uninit_applicant() only holds rtnl lock which is not
> enough here.
> 
> Found by code inspection.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Ward <david.ward@ll.mit.edu>
> Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

Under what conditions can entries be removed or added to
these RB-trees without the RTNL being held?

If such events cannot happen, then no locking is needed.

Even if your change is correct and necessary, the answer to my
needs to be added to your commit message.

Thanks.

Re: [PATCH net-next] 802: fix a possible race condition

[Cong Wang]

On Sun, 2013-03-24 at 17:24 -0400, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Sat, 23 Mar 2013 13:14:08 +0800
> 
> > From: Cong Wang <amwang@redhat.com>
> > 
> > garp_pdu_queue() should ways be called with this spin lock.
> > garp_uninit_applicant() only holds rtnl lock which is not
> > enough here.
> > 
> > Found by code inspection.
> > 
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: David Ward <david.ward@ll.mit.edu>
> > Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> Under what conditions can entries be removed or added to
> these RB-trees without the RTNL being held?

At least garp_join_timer() calls garp_pdu_queue() in a timer:

static void garp_join_timer(unsigned long data)
{
        struct garp_applicant *app = (struct garp_applicant *)data;

        spin_lock(&app->lock);
        garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
        garp_pdu_queue(app);
        spin_unlock(&app->lock);

        garp_queue_xmit(app);
        garp_join_timer_arm(app);
}

which I don't think can hold RTNL lock possibly.

> 
> If such events cannot happen, then no locking is needed.
> 
> Even if your change is correct and necessary, the answer to my
> needs to be added to your commit message.

Ok, I will.

Thanks.

Re: [PATCH net-next] 802: fix a possible race condition

[Eric Dumazet]

On Mon, 2013-03-25 at 21:32 +0800, Cong Wang wrote:

> At least garp_join_timer() calls garp_pdu_queue() in a timer:
> 
> static void garp_join_timer(unsigned long data)
> {
>         struct garp_applicant *app = (struct garp_applicant *)data;
> 
>         spin_lock(&app->lock);
>         garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
>         garp_pdu_queue(app);
>         spin_unlock(&app->lock);
> 
>         garp_queue_xmit(app);
>         garp_join_timer_arm(app);
> }
> 
> which I don't think can hold RTNL lock possibly.
> 

But timer wont possibly run because of the previous :

del_timer_sync(&app->join_timer);

Re: [PATCH net-next] 802: fix a possible race condition

[Cong Wang]

On Mon, 2013-03-25 at 07:08 -0700, Eric Dumazet wrote:
> On Mon, 2013-03-25 at 21:32 +0800, Cong Wang wrote:
> 
> > At least garp_join_timer() calls garp_pdu_queue() in a timer:
> > 
> > static void garp_join_timer(unsigned long data)
> > {
> >         struct garp_applicant *app = (struct garp_applicant *)data;
> > 
> >         spin_lock(&app->lock);
> >         garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
> >         garp_pdu_queue(app);
> >         spin_unlock(&app->lock);
> > 
> >         garp_queue_xmit(app);
> >         garp_join_timer_arm(app);
> > }
> > 
> > which I don't think can hold RTNL lock possibly.
> > 
> 
> But timer wont possibly run because of the previous :
> 
> del_timer_sync(&app->join_timer);

Yeah, but in the following callchain:

garp_pdu_rcv() -> garp_pdu_parse_msg() -> garp_pdu_parse_attr() ->
garp_gid_event()

the race can happen too as garp_pdu_rcv() is called in BH context.

[PATCH net-next] 802: fix a possible race condition

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

(Resend with a better changelog)

garp_pdu_queue() should ways be called with this spin lock.
garp_uninit_applicant() only holds rtnl lock which is not
enough here.  A possible race can happen as garp_pdu_rcv()
is called in BH context:

	garp_pdu_rcv()
	  |->garp_pdu_parse_msg()
	    |->garp_pdu_parse_attr()
	      |-> garp_gid_event()

Found by code inspection.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ward <david.ward@ll.mit.edu>
Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/802/garp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8456f5d..5d9630a 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -609,8 +609,12 @@ void garp_uninit_applicant(struct net_device *dev, struct garp_application *appl
 	/* Delete timer and generate a final TRANSMIT_PDU event to flush out
 	 * all pending messages before the applicant is gone. */
 	del_timer_sync(&app->join_timer);
+
+	spin_lock_bh(&app->lock);
 	garp_gid_event(app, GARP_EVENT_TRANSMIT_PDU);
 	garp_pdu_queue(app);
+	spin_unlock_bh(&app->lock);
+
 	garp_queue_xmit(app);
 
 	dev_mc_del(dev, appl->proto.group_address);
-- 
1.7.7.6

Re: [PATCH net-next] 802: fix a possible race condition

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Wed,  3 Apr 2013 15:52:40 +0800

> From: Cong Wang <amwang@redhat.com>
> 
> (Resend with a better changelog)
> 
> garp_pdu_queue() should ways be called with this spin lock.
> garp_uninit_applicant() only holds rtnl lock which is not
> enough here.  A possible race can happen as garp_pdu_rcv()
> is called in BH context:
> 
> 	garp_pdu_rcv()
> 	  |->garp_pdu_parse_msg()
> 	    |->garp_pdu_parse_attr()
> 	      |-> garp_gid_event()
> 
> Found by code inspection.
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Ward <david.ward@ll.mit.edu>
> Cc: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

Applied.