netdev - Re: [PATCH 4/6] d80211: add IEEE802.11e/WMM Traffic Stream (TS) Management support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1166194595.3462.19.camel@johannes.berg>
Date:	Fri, 15 Dec 2006 15:56:35 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	yi.zhu@...el.com
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 4/6] d80211: add IEEE802.11e/WMM Traffic Stream (TS)
	Management support

Some comments...

In these cases 
 
> +		init_timer(&ifsta->admit_timer);
> +		ifsta->admit_timer.data = (unsigned long) dev;
> +		ifsta->admit_timer.function = ieee80211_admit_refresh;

> +void ieee80211_send_addts(struct net_device *dev,
> +                         struct ieee802_11_elem_tspec *tspec)

> +void wmm_send_addts(struct net_device *dev, struct ieee802_11_elem_tspec *tspec)

> +void ieee80211_send_delts(struct net_device *dev, u8 tsid, u8 direction,
> +                         u32 medium_time)

> +void wmm_send_delts(struct net_device *dev, u8 tsid, u8 direction,
> +                   u32 medium_time)

please don't use the device as the argument but rather the sdata. Using
the device only to unwrap it to the sdata again isn't really nice.

> +	mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> +	memset(mgmt, 0, 24);
> +	memcpy(mgmt->da, ifsta->bssid, ETH_ALEN);
> +	memcpy(mgmt->sa, dev->dev_addr, ETH_ALEN);
> +	memcpy(mgmt->bssid, ifsta->bssid, ETH_ALEN);

No need to zero out the structure if you set all fields anyway.

> +	mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> +	memset(mgmt, 0, 24);
> +	memcpy(mgmt->da, ifsta->bssid, ETH_ALEN);
> +	memcpy(mgmt->sa, dev->dev_addr, ETH_ALEN);
> +	memcpy(mgmt->bssid, ifsta->bssid, ETH_ALEN);
> +	mgmt->frame_control = IEEE80211_FC(IEEE80211_FTYPE_MGMT,
> +					   IEEE80211_STYPE_ACTION);

same here.

> +	mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> +	memset(mgmt, 0, 24);

and here, and in a few more places. 
 
> +static u32 calculate_mpdu_exchange_time(struct ieee802_11_elem_tspec *tspec)
> +{
> +	/*
> +	 * MPDUExchangeTime = duration(Nominal MSDU Size, Min PHY Rate) +
> +	 *		      SIFS + ACK duration
> +	 */
> +	return 5000;
> +}

Is this correct? Aren't some of those things variable?

> +	printk(KERN_DEBUG "Dialog_token: %d, TID: %u, Direction: %u, PSB: %d, "
> +	       "UP: %d\n", mgmt->u.action.u.wme_action.dialog_token,
> +	       tspec->ts_info.tsid, tspec->ts_info.direction,
> +	       tspec->ts_info.apsd, tspec->ts_info.up);

Can we have those printks optional?

> +static void ieee80211_rx_mgmt_action(struct net_device *dev,

> +	if (len < 24 + 1) {
> +		printk(KERN_DEBUG "%s: too short (%zd) action frame "
> +		       "received from " MAC_FMT " - ignored\n",
> +		       dev->name, len, MAC_ARG(mgmt->sa));
> +		return;
> +	}

Do we really want this if in all possibilities where we use the data it
needs to be 24+4 long?

> +	case WLAN_CATEGORY_DLS:
> +	case WLAN_CATEGORY_BACK:
> +	default:
> +		printk(KERN_ERR "%s: unsupported action category %d\n",
> +		       dev->name, mgmt->u.action.category);
> +		break;

I don't think these are KERN_ERR. 
 
johannes

Download attachment "signature.asc" of type "application/pgp-signature" (191 bytes)