| Message ID | 1364015648-4195-2-git-send-email-amwang@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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. -- 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
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); -- 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
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. -- 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/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);