lkml.org 
[lkml]   [2014]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 1/4] mac80211: OCB mode + join and leave handling
From
Date
Hello Johannes;
Thanks for the thorough review.

On Thu, 2014-10-09 at 10:23 +0200, Johannes Berg wrote:
> On Thu, 2014-09-11 at 16:30 +0200, Rostislav Lisovy wrote:
> > +++ b/net/mac80211/cfg.c
> > @@ -229,6 +229,7 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
> > case NUM_NL80211_IFTYPES:
> > case NL80211_IFTYPE_P2P_CLIENT:
> > case NL80211_IFTYPE_P2P_GO:
> > + case NL80211_IFTYPE_OCB:
> > /* shouldn't happen */
>
> There's no encryption in OCB at all?

As far as I know the standard 802.11* encryption is not used. The IEEE
1609 (WAVE protocol stack used in US) does define some encryption but it
is not part of the 802.11p.

> > +void ieee80211_ocb_rx_no_sta(struct ieee80211_sub_if_data *sdata,
> > + const u8 *bssid, const u8 *addr,
> > + u32 supp_rates)
>
> Does this have to be visible outside the file? I may have missed the
> reference(s) but it seems maybe it doesn't have to.
>

Please see below.

> > + mutex_lock(&sdata->local->mtx);
> > + ieee80211_vif_release_channel(sdata);
> > + mutex_unlock(&sdata->local->mtx);
> > +
> > + skb_queue_purge(&sdata->skb_queue);
> > +
> > + del_timer_sync(&sdata->u.ocb.housekeeping_timer);
>
> That might call the timer - is it safe if that happens here? Looks like
> maybe the housekeeping would still get triggered or so.

You are right. I hope the following is a reasonable solution (in form of
a patch to my previous patch; comment stolen from some prehistoric
version of mesh.c):

@@ -127,6 +127,9 @@ void ieee80211_ocb_work(struct ieee80211_sub_if_data *sdata)
struct ieee80211_if_ocb *ifocb = &sdata->u.ocb;
struct sta_info *sta;

+ if (!netif_running(sdata->dev))
+ return;
+
sdata_lock(sdata);

spin_lock_bh(&ifocb->incomplete_lock);
@@ -229,6 +232,13 @@ int ieee80211_ocb_leave(struct ieee80211_sub_if_data *sdata)
skb_queue_purge(&sdata->skb_queue);

del_timer_sync(&sdata->u.ocb.housekeeping_timer);
+ /*
+ * If the timer fired while we waited for it, it will have
+ * requeued the work. Now the work will be running again
+ * but will not rearm the timer again because it checks
+ * whether the interface is running, which, at this point,
+ * it no longer is.
+ */

return 0;
}

> > + } else if (!multicast &&
> > + !ether_addr_equal(sdata->dev->dev_addr, hdr->addr1)) {
> > + /* if we are in promisc mode we also accept
> > + * packets not destined for us
> > + */
> > + if (!(sdata->dev->flags & IFF_PROMISC))
> > + return false;
> > + rx->flags &= ~IEEE80211_RX_RA_MATCH;
> > + } else if (!rx->sta) {
> > + int rate_idx;
> > + if (status->flag & RX_FLAG_HT)
> > + rate_idx = 0; /* TODO: HT rates */
> > + else
> > + rate_idx = status->rate_idx;
> > + ieee80211_ocb_rx_no_sta(sdata, bssid, hdr->addr2,
> > + BIT(rate_idx));
> > + }
>
> This isn't safe - ocb_rx_no_sta() used GFP_KERNEL, that's clearly not
> allowed in this context. But it does answer my previous question about
> the function being exported - I had assumed that you wouldn't call it
> here since it would be unsafe :)

A call to sta_info_alloc(sdata, addr, GFP_ATOMIC);
in ieee80211_ocb_rx_no_sta() should solve this.


I agree with all the other comments and will fix them.

Best regards;
Rostislav;



\
 
 \ /
  Last update: 2014-10-16 19:01    [W:1.581 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site