On Wed, 2007-06-06 at 21:18 +0200, Johannes Berg wrote:
> Some nitpicking below :)
>
> > + if (dls_link_status(local, skb->data) == DLS_STATUS_OK){
>
> missing space there, but maybe the line is too long with it?
Yup. The line happens to be 80 originally before Michael pointed a bug
here. I forgot to add the space back after that.
> > + case WIFI_OUI_STYPE_WMM_TSPEC:
> > + if (elen != 61) {
> > + printk(KERN_ERR "Wrong "
> > + "TSPEC size.\n");
> > + break;
>
> That printk should be rate limited.
Umm, I followed the same way as other MLME parsing functions ie.
ieee80211_rx_mgmt_assoc_resp. It seems they also didn't rate limit these
cases. This only happens on broken APs. Do you think we really need to
rate limit here?
> > + default:
> > + //printk(KERN_ERR "Unsupported "
> > + // "WiFi OUI %d\n", pos[4]);
> > + break;
>
> And maybe that should be there for the debug case (rate limited as
> well)?
I got a lot of this message during my testing. The APs use some
(private) OUIs we don't support now. I guess there will be quite a lot
messages even if we rate limit here.
> > + case WLAN_EID_TSPEC:
> > + if (elen != 55) {
> > + printk(KERN_ERR "Wrong TSPEC size.\n");
> > + break;
>
> Ditto.
>
> > + if (ifsta->ts_data[tsid][index].status == TS_STATUS_UNUSED) {
> > + printk(KERN_DEBUG "%s: Tring to delete an ACM disabled TS "
>
> typo "Tring".
Thanks!
> Can this be invoked by somebody requesting you to do it? If so it should
> be rate limited as well. I haven't quite understood the code flow here
> yet.
>
> > + if (ieee802_11_parse_elems(pos, len - (pos - (u8 *) mgmt), &elems)
> > + == ParseFailed) {
> > + printk(KERN_DEBUG "%s: failed to parse TSPEC\n", dev->name);
> > + return;
>
> Wants rate limiting too, afaict.
mac80211 doesn't rate limit ParseFailed cases in other parts. ie.
ieee80211_rx_mgmt_assoc_resp, etc.
> > + printk(KERN_DEBUG "Receive DLS request from "
> > + "%02X:%02X:%02X:%02X:%02X:%02X\n",
> > + src[0], src[1], src[2], src[3], src[4], src[5]);
>
> Again?
This management packet is sent from the AP. Not from the STA user
requests DLS connection directly. So can I compare this to the following
line in ieee80211_rx_mgmt_assoc_resp().
printk(KERN_DEBUG "%s: RX %sssocResp from " MAC_FMT " (capab=0x%x "
"status=%d aid=%d)\n",
dev->name, reassoc ? "Rea" : "A", MAC_ARG(mgmt->sa),
capab_info, status_code, aid);
> > + if (ieee802_11_parse_elems(mgmt->u.action.u.dls_req.variable,
> > + len - baselen, &elems) == ParseFailed) {
> > + printk(KERN_ERR "DLS Parse support rates failed.\n");
> > + return;
>
> and again?
See above.
> > + printk(KERN_DEBUG "Receive DLS response from "
> > + "%02X:%02X:%02X:%02X:%02X:%02X\n",
> > + src[0], src[1], src[2], src[3], src[4], src[5]);
> > +
> > + if (mgmt->u.action.u.dls_resp.status_code) {
> > + printk(KERN_ERR "DLS setup refused by peer. Reason %d\n",
> > + mgmt->u.action.u.dls_resp.status_code);
> > + return;
> > + }
>
> I suppose these are ok since we're not going to request DLS a lot,
> right?
Yup.
> > + printk(KERN_DEBUG "DLS Teardown received from "
> > + "%02X:%02X:%02X:%02X:%02X:%02X. Reason %d\n",
> > + src[0], src[1], src[2], src[3], src[4], src[5],
> > + mgmt->u.action.u.dls_teardown.reason_code);
> > +
> > + dls = dls_info_get(local, src);
> > + if (dls)
> > + sta_info_free(dls, 0);
>
> Do we really want to free the sta info here? Shouldn't we have a sta
> info item for the DLS peer anyway, and then we can't free it here but
> rather reset the DLS status?
Yes we can do this. But free it here helps to shrink the hash table
size. The problem is: the original sta_info doesn't a status -- if a
sta_info appears in the hash table, it is active; otherwise it is not.
But since DLS requires setup stage, I added the status code to
distinguish setuping and active. But for inactive state, we can either
set the DLS status to inactive or remove the sta_info completely. Since
normally a DLS connection won't be setup and teardown that often, I
select to free the sta_info to indicate it as inactive.
> > + if (len < 24 + 4) {
> > + printk(KERN_DEBUG "%s: too short (%zd) QoS category "
> > + "frame received from " MAC_FMT " - ignored\n",
> > + dev->name, len, MAC_ARG(mgmt->sa));
>
> rate limit.
See the following code in ieee80211_rx_mgmt_auth()
if (len < 24 + 6) {
printk(KERN_DEBUG "%s: too short (%zd) authentication
frame "
"received from " MAC_FMT " - ignored\n",
dev->name, len, MAC_ARG(mgmt->sa));
return;
}
> > + printk(KERN_ERR "%s: unsupported QoS action code %d\n",
> > + dev->name,
> > + mgmt->u.action.u.wme_action.action_code);
>
> rate limit.
There is no other QoS action code not supported by mac80211. If we see
any, we can fix it.
> > + printk(KERN_DEBUG "%s: too short (%zd) DLS category "
> > + "frame received from " MAC_FMT " - ignored\n",
> > + dev->name, len, MAC_ARG(mgmt->sa));
>
> ditto.
see above.
>
> > + printk(KERN_ERR "%s: unsupported DLS action code %d\n",
> > + dev->name, mgmt->u.action.u.dls_req.action_code);
>
> ditto.
see above.
> > +struct sta_info *dls_info_get(struct ieee80211_local *local, u8 *addr)
>
> Ok, I don't understand the DLS info stuff. Basically it's a sta info,
> but why not just do sta_info_get() on the regular one and then set the
> dls peer bit?
Do you mean below code instead?
struct sta_info *dls_info_get()
{
sta = sta_info_get(local, addr);
if (sta)
if (sta->dls_sta)
return sta;
else
sta_info_put(sta);
return NULL;
}
This is not optimized in the (sta && !sta->dls_sta) case.
> > + if (net_ratelimit())
> > + printk(KERN_DEBUG "QoS packet throttling\n");
>
> Hey! :)
Yeah, that's a hot path.
Thanks for your feedbacks!
-yi
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
