| Message ID | 1367548442-8229-7-git-send-email-thomas@cozybit.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, 2013-05-02 at 19:33 -0700, Thomas Pedersen wrote: > After turning carrier off, any parent bridge interface > needs to be notified. Otherwise we would see a panic when > attempting to transmit frames on a mesh interface which > hadn't yet been put down. > > Signed-off-by: Thomas Pedersen <thomas@cozybit.com> > --- > net/mac80211/mesh.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c > index eac9988..271ddc9 100644 > --- a/net/mac80211/mesh.c > +++ b/net/mac80211/mesh.c > @@ -993,6 +993,9 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata) > struct beacon_data *bcn; > > netif_carrier_off(sdata->dev); > + if (sdata->dev->priv_flags & IFF_BRIDGE_PORT) > + /* stop bridge transmissions */ > + call_netdevice_notifiers(NETDEV_CHANGE, sdata->dev); Err, this seems like a really bad hack? I don't really think drivers should call that? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, Sorry for the late response. On Tue, May 7, 2013 at 6:42 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2013-05-02 at 19:33 -0700, Thomas Pedersen wrote: >> After turning carrier off, any parent bridge interface >> needs to be notified. Otherwise we would see a panic when >> attempting to transmit frames on a mesh interface which >> hadn't yet been put down. >> >> Signed-off-by: Thomas Pedersen <thomas@cozybit.com> >> --- >> net/mac80211/mesh.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c >> index eac9988..271ddc9 100644 >> --- a/net/mac80211/mesh.c >> +++ b/net/mac80211/mesh.c >> @@ -993,6 +993,9 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata) >> struct beacon_data *bcn; >> >> netif_carrier_off(sdata->dev); >> + if (sdata->dev->priv_flags & IFF_BRIDGE_PORT) >> + /* stop bridge transmissions */ >> + call_netdevice_notifiers(NETDEV_CHANGE, sdata->dev); > > Err, this seems like a really bad hack? I don't really think drivers > should call that? Why not? We're just notifying the bridge interface that this port has gone down, to avoid dereferencing a null pointer (on bridge flood traffic) after the mesh_bss has been removed. Is cfg80211_leave_mesh() an acceptable location for this fix? -- Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-05-21 at 18:09 -0700, Thomas Pedersen wrote: > Hey, Sorry for the late response. Me too :) > >> netif_carrier_off(sdata->dev); > >> + if (sdata->dev->priv_flags & IFF_BRIDGE_PORT) > >> + /* stop bridge transmissions */ > >> + call_netdevice_notifiers(NETDEV_CHANGE, sdata->dev); > > > > Err, this seems like a really bad hack? I don't really think drivers > > should call that? > > Why not? We're just notifying the bridge interface that this port has > gone down, to avoid dereferencing a null pointer (on bridge flood > traffic) after the mesh_bss has been removed. > > Is cfg80211_leave_mesh() an acceptable location for this fix? I'm not really convinced it's an acceptable fix in itself? Why should we ever have to call netdev notifiers, that seems odd to me. Also why would that prevent a crash? Wouldn't we just drop packets for the mesh if we aren't joined in a mesh any more? Or something like that? Accessing the priv flags and then calling the netdev notifiers seems really strange to me. If this was necessary, then wouldn't netif_carrier_off() do it internally? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 24, 2013 at 1:32 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Tue, 2013-05-21 at 18:09 -0700, Thomas Pedersen wrote: >> Hey, Sorry for the late response. > > Me too :) > >> >> netif_carrier_off(sdata->dev); >> >> + if (sdata->dev->priv_flags & IFF_BRIDGE_PORT) >> >> + /* stop bridge transmissions */ >> >> + call_netdevice_notifiers(NETDEV_CHANGE, sdata->dev); >> > >> > Err, this seems like a really bad hack? I don't really think drivers >> > should call that? >> >> Why not? We're just notifying the bridge interface that this port has >> gone down, to avoid dereferencing a null pointer (on bridge flood >> traffic) after the mesh_bss has been removed. >> >> Is cfg80211_leave_mesh() an acceptable location for this fix? > > I'm not really convinced it's an acceptable fix in itself? Why should we > ever have to call netdev notifiers, that seems odd to me. Also why would > that prevent a crash? Wouldn't we just drop packets for the mesh if we > aren't joined in a mesh any more? Or something like that Accessing the > priv flags and then calling the netdev notifiers seems really strange to > me. If this was necessary, then wouldn't netif_carrier_off() do it > internally? OK I'll take a look at this and figure out a cleaner solution then. Thanks. -- Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 28, 2013 at 10:01 AM, Thomas Pedersen <thomas@cozybit.com> wrote: > On Fri, May 24, 2013 at 1:32 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: >> On Tue, 2013-05-21 at 18:09 -0700, Thomas Pedersen wrote: >>> Hey, Sorry for the late response. >> >> Me too :) >> >>> >> netif_carrier_off(sdata->dev); >>> >> + if (sdata->dev->priv_flags & IFF_BRIDGE_PORT) >>> >> + /* stop bridge transmissions */ >>> >> + call_netdevice_notifiers(NETDEV_CHANGE, sdata->dev); >>> > >>> > Err, this seems like a really bad hack? I don't really think drivers >>> > should call that? >>> >>> Why not? We're just notifying the bridge interface that this port has >>> gone down, to avoid dereferencing a null pointer (on bridge flood >>> traffic) after the mesh_bss has been removed. >>> >>> Is cfg80211_leave_mesh() an acceptable location for this fix? >> >> I'm not really convinced it's an acceptable fix in itself? Why should we >> ever have to call netdev notifiers, that seems odd to me. Also why would >> that prevent a crash? Wouldn't we just drop packets for the mesh if we >> aren't joined in a mesh any more? Or something like that Accessing the >> priv flags and then calling the netdev notifiers seems really strange to >> me. If this was necessary, then wouldn't netif_carrier_off() do it >> internally? > > OK I'll take a look at this and figure out a cleaner solution then. Thanks. After reverting this patch I can no longer trigger the crash it was supposed to fix. Either something changed in the rebase, or it was never needed. Will drop from the resubmission for now. Thanks, -- Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/mac80211/mesh.c b/net/mac80211/mesh.c index eac9988..271ddc9 100644 --- a/net/mac80211/mesh.c +++ b/net/mac80211/mesh.c @@ -993,6 +993,9 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata) struct beacon_data *bcn; netif_carrier_off(sdata->dev); + if (sdata->dev->priv_flags & IFF_BRIDGE_PORT) + /* stop bridge transmissions */ + call_netdevice_notifiers(NETDEV_CHANGE, sdata->dev); /* stop the beacon */ ifmsh->mesh_id_len = 0;
After turning carrier off, any parent bridge interface needs to be notified. Otherwise we would see a panic when attempting to transmit frames on a mesh interface which hadn't yet been put down. Signed-off-by: Thomas Pedersen <thomas@cozybit.com> --- net/mac80211/mesh.c | 3 +++ 1 file changed, 3 insertions(+)