On Thu, 2014-09-11 at 16:30 +0200, Rostislav Lisovy wrote:
[...]
Nice work. Let me (finally) look in more detail ...
> +++ b/include/uapi/linux/nl80211.h
> @@ -2031,6 +2031,8 @@ enum nl80211_attrs {
> * and therefore can't be created in the normal ways, use the
> * %NL80211_CMD_START_P2P_DEVICE and %NL80211_CMD_STOP_P2P_DEVICE
> * commands to create and destroy one
> + * @NL80211_IF_TYPE_OCB: Outside Context of a BSS
> + * this mode corresponds to the MIB variable dot11OCBActivated=true
This change should be part of the cfg80211 patch, as a consequence that
patch must come first in the series.
> +++ 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?
> +++ b/net/mac80211/ieee80211_i.h
> @@ -82,6 +82,8 @@ struct ieee80211_fragment_entry {
> u8 last_pn[6]; /* PN of the last fragment if CCMP was used */
> };
>
> +/* Used in OCB mode */
> +static const u8 bssid_wildcard[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
Please don't put data into header files, it'll be duplicated all over.
You can put an extern definition here, or just static const
definition(s) where needed.
> +struct ieee80211_if_ocb {
> + struct timer_list housekeeping_timer;
> + unsigned long wrkq_flags;
> +
> + spinlock_t incomplete_lock;
> + struct list_head incomplete_stations;
> +};
Please add some comments explaining the purpose of the fields.
> +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.
> +/* TODO: Almost the same as ieee80211_ibss_finish_sta()
> + * Maybe use the same function for both?
> + */
> +static struct sta_info *ieee80211_ocb_finish_sta(struct sta_info *sta)
> + __acquires(RCU)
> +{
> + struct ieee80211_sub_if_data *sdata = sta->sdata;
> + u8 addr[ETH_ALEN];
> +
> + ether_addr_copy(addr, sta->sta.addr);
for ether_addr_copy() you need alignment on addr:
u8 addr[ETH_ALEN] __aligned(2);
> +int ieee80211_ocb_join(struct ieee80211_sub_if_data *sdata,
> + struct ocb_setup *setup)
> +{
> + struct ieee80211_local *local = sdata->local;
> + struct ieee80211_if_ocb *ifocb = &sdata->u.ocb;
> + u32 changed = BSS_CHANGED_OCB;
> + int err;
> +
> + sdata->flags |= IEEE80211_SDATA_OPERATING_GMODE;
> + sdata->smps_mode = IEEE80211_SMPS_OFF;
> + sdata->needed_rx_chains = sdata->local->rx_chains;
> +
> + mutex_lock(&sdata->local->mtx);
> + err = ieee80211_vif_use_channel(sdata, &setup->chandef,
> + IEEE80211_CHANCTX_EXCLUSIVE);
Do you really need to use the channel context exclusively? I see no
reason for that since the channel is fixed, and doesn't change like in
IBSS, afaict? IBSS needs this, but only because it might change the
channel on the fly.
> +#if 0 /* TODO */
> + drv_leave_ocb(local, sdata);
> +#endif
I'd just remove that for now - probably no current driver needs it?
> + 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.
> + } else if (rx->sdata->vif.type == NL80211_IFTYPE_OCB) {
> + u8 *bssid = ieee80211_get_bssid(hdr, rx->skb->len,
> + NL80211_IFTYPE_OCB);
> + if (ieee80211_bssid_match(bssid, bssid_wildcard))
> + sta->last_rx = jiffies;
Calling ieee80211_bssid_match() here is rather pointless - see what it
does. You really just want is_broadcast_ether_addr(bssid) instead of the
ether_addr_equal() that does exactly the same thing.
> @@ -3130,6 +3137,32 @@ static bool prepare_for_handlers(struct ieee80211_rx_data *rx,
> BIT(rate_idx));
> }
> break;
> + case NL80211_IFTYPE_OCB:
> + if (!bssid)
> + return false;
> + if (ieee80211_is_beacon(hdr->frame_control)) {
> + return false;
> + } else if (!ieee80211_bssid_match(bssid, bssid_wildcard)) {
> + ocb_dbg(sdata, "BSSID mismatch in OCB mode!\n");
> + return false;
same here.
And then I guess you don't need bssid_wildcard at all any more (see
previous comments)
> + } 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 :)
> + case NL80211_IFTYPE_OCB:
> + /* DA SA BSSID */
> + memcpy(hdr.addr1, skb->data, ETH_ALEN);
> + memcpy(hdr.addr2, skb->data + ETH_ALEN, ETH_ALEN);
> + memset(hdr.addr3, 0xff, ETH_ALEN);
Use eth_broadcast_addr(), which really is the same but more expressive.
johannes
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;
On Thu, 2014-10-16 at 18:33 +0200, Rostislav Lisovy wrote:
> > > + 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;
> }
Now I realized it is not that easy (and I confused "interface running"
and being "connected to the network"). There seems not to be a solid
indication that we are no longer "connected" to the network.
I think a field
enum {
IEEE80211_OCB_STOPPED,
IEEE80211_OCB_JOINED,
} state;
in struct ieee80211_if_ocb seems to be appropriate.
Best regards;
Rostislav
On Thu, 2014-10-16 at 18:33 +0200, Rostislav Lisovy wrote:
> 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;
Not sure, it seems you should check "is it operating in OCB mode"? OTOH,
when it's not operating but still around it probably doesn't matter?
> @@ -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.
> + */
Well, the comment is wrong, since leave() can and will be done while the
interface is running.
> > 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.
Yeah, I guess so, didn't check in detail now.
johannes
On Thu, 2014-10-16 at 19:20 +0200, Rostislav Lisovy wrote:
> Now I realized it is not that easy (and I confused "interface running"
> and being "connected to the network"). There seems not to be a solid
> indication that we are no longer "connected" to the network.
Oops, ignore my previous reply.
> I think a field
> enum {
> IEEE80211_OCB_STOPPED,
> IEEE80211_OCB_JOINED,
> } state;
> in struct ieee80211_if_ocb seems to be appropriate.
Yes, that seems doable. Maybe just a "bool joined" would work just as
well, but it won't make a difference in the result - whichever you
prefer.
johannes