[PATCH 1/1] mac80211: restructuring data Rx handlers

[PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

This patch restructures the Rx handlers chain by incorporating previously
handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
precisely after the IEEE802.11 data plane archituecture, and will prevent
code duplication to IEEE8021.11n A-MSDU handler.

added function:
 - ieee80211_data_to_8023: transfering 802.11 data frames to 802.3 frame
 - ieee80211_deliver_skb: delivering the 802.3 frames to upper stack
eliminated handlers:
 - ieee80211_rx_h_drop_unencrypted: now function ieee80211_drop_unencrypted
 - ieee80211_rx_h_802_1x_pae: now function ieee80211_802_1x_pae
changed handlers:
 - ieee80211_rx_h_data: now contains calls to four above function

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/ieee80211_i.h |    2 +-
 net/mac80211/rx.c          |  124 +++++++++++++++++++++++++++-----------------
 net/mac80211/tx.c          |   12 +++--
 net/mac80211/util.c        |   14 +-----
 4 files changed, 86 insertions(+), 66 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b4e32ab..0444809 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -792,7 +792,7 @@ extern void *mac80211_wiphy_privid; /* for wiphy privid */
 extern const unsigned char rfc1042_header[6];
 extern const unsigned char bridge_tunnel_header[6];
 u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
-int ieee80211_is_eapol(const struct sk_buff *skb);
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
 int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,
 			     int rate, int erp, int short_preamble);
 void mac80211_ev_michael_mic_failure(struct net_device *dev, int keyidx,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 428a9fc..6333c98 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -956,68 +956,64 @@ ieee80211_rx_h_remove_qos_control(struct ieee80211_txrx_data *rx)
 	return TXRX_CONTINUE;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_drop_802_1x_pae(struct ieee80211_txrx_data *rx, int hdrlen)
 {
-	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
+	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&
 	    rx->sdata->type != IEEE80211_IF_TYPE_STA &&
 	    (rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
-		return TXRX_CONTINUE;
+		return 0;
 
 	if (unlikely(rx->sdata->ieee802_1x &&
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     (!rx->sta || !(rx->sta->flags & WLAN_STA_AUTHORIZED)) &&
-		     !ieee80211_is_eapol(rx->skb))) {
+		     !ieee80211_is_eapol(rx->skb, hdrlen))) {
 #ifdef CONFIG_MAC80211_DEBUG
-		struct ieee80211_hdr *hdr =
-			(struct ieee80211_hdr *) rx->skb->data;
-		DECLARE_MAC_BUF(mac);
-		printk(KERN_DEBUG "%s: dropped frame from %s"
-		       " (unauthorized port)\n", rx->dev->name,
-		       print_mac(mac, hdr->addr2));
+		printk(KERN_DEBUG "%s: dropped frame "
+		       "(unauthorized port)\n", rx->dev->name);
 #endif /* CONFIG_MAC80211_DEBUG */
-		return TXRX_DROP;
+		return -EACCES;
 	}
 
-	return TXRX_CONTINUE;
+	return 0;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_drop_unencrypted(struct ieee80211_txrx_data *rx, int hdrlen)
 {
 	/*
 	 * Pass through unencrypted frames if the hardware has
 	 * decrypted them already.
 	 */
 	if (rx->u.rx.status->flag & RX_FLAG_DECRYPTED)
-		return TXRX_CONTINUE;
+		return 0;
 
 	/* Drop unencrypted frames if key is set. */
 	if (unlikely(!(rx->fc & IEEE80211_FCTL_PROTECTED) &&
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     rx->sdata->drop_unencrypted &&
-		     (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
+		     (!rx->sdata->eapol ||
+		      !ieee80211_is_eapol(rx->skb, hdrlen)))) {
 		if (net_ratelimit())
 			printk(KERN_DEBUG "%s: RX non-WEP frame, but expected "
 			       "encryption\n", rx->dev->name);
-		return TXRX_DROP;
+		return -EACCES;
 	}
-	return TXRX_CONTINUE;
+	return 0;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_data_to_8023(struct ieee80211_txrx_data *rx)
 {
 	struct net_device *dev = rx->dev;
-	struct ieee80211_local *local = rx->local;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
 	u16 fc, hdrlen, ethertype;
 	u8 *payload;
 	u8 dst[ETH_ALEN];
 	u8 src[ETH_ALEN];
-	struct sk_buff *skb = rx->skb, *skb2;
+	struct sk_buff *skb = rx->skb;
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	DECLARE_MAC_BUF(mac);
 	DECLARE_MAC_BUF(mac2);
@@ -1025,11 +1021,9 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 	DECLARE_MAC_BUF(mac4);
 
 	fc = rx->fc;
-	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
-		return TXRX_CONTINUE;
 
 	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return TXRX_DROP;
+		return -1;
 
 	hdrlen = ieee80211_get_hdrlen(fc);
 
@@ -1058,7 +1052,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac, hdr->addr1),
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3));
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	case (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS):
@@ -1075,7 +1069,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3),
 				       print_mac(mac4, hdr->addr4));
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	case IEEE80211_FCTL_FROMDS:
@@ -1086,7 +1080,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		if (sdata->type != IEEE80211_IF_TYPE_STA ||
 		    (is_multicast_ether_addr(dst) &&
 		     !compare_ether_addr(src, dev->dev_addr)))
-			return TXRX_DROP;
+			return -1;
 		break;
 	case 0:
 		/* DA SA BSSID */
@@ -1102,21 +1096,20 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3));
 			}
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	}
 
-	payload = skb->data + hdrlen;
-
 	if (unlikely(skb->len - hdrlen < 8)) {
 		if (net_ratelimit()) {
 			printk(KERN_DEBUG "%s: RX too short data frame "
 			       "payload\n", dev->name);
 		}
-		return TXRX_DROP;
+		return -1;
 	}
 
+	payload = skb->data + hdrlen;
 	ethertype = (payload[6] << 8) | payload[7];
 
 	if (likely((compare_ether_addr(payload, rfc1042_header) == 0 &&
@@ -1137,12 +1130,19 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		memcpy(ehdr->h_source, src, ETH_ALEN);
 		ehdr->h_proto = len;
 	}
-	skb->dev = dev;
+	return 0;
+}
 
-	skb2 = NULL;
+static void
+ieee80211_deliver_skb(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	struct ieee80211_local *local = rx->local;
+	struct sk_buff *skb, *xmit_skb;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
-	dev->stats.rx_packets++;
-	dev->stats.rx_bytes += skb->len;
+	skb = rx->skb;
+	xmit_skb = NULL;
 
 	if (local->bridge_packets && (sdata->type == IEEE80211_IF_TYPE_AP
 	    || sdata->type == IEEE80211_IF_TYPE_VLAN) &&
@@ -1150,8 +1150,8 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		if (is_multicast_ether_addr(skb->data)) {
 			/* send multicast frames both to higher layers in
 			 * local net stack and back to the wireless media */
-			skb2 = skb_copy(skb, GFP_ATOMIC);
-			if (!skb2 && net_ratelimit())
+			xmit_skb = skb_copy(skb, GFP_ATOMIC);
+			if (!xmit_skb && net_ratelimit())
 				printk(KERN_DEBUG "%s: failed to clone "
 				       "multicast frame\n", dev->name);
 		} else {
@@ -1166,7 +1166,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				 * AP, so send the frame directly to it and
 				 * do not pass the frame to local net stack.
 				 */
-				skb2 = skb;
+				xmit_skb = skb;
 				skb = NULL;
 			}
 			if (dsta)
@@ -1181,13 +1181,45 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		netif_rx(skb);
 	}
 
-	if (skb2) {
+	if (xmit_skb) {
 		/* send to wireless media */
-		skb2->protocol = __constant_htons(ETH_P_802_3);
-		skb_set_network_header(skb2, 0);
-		skb_set_mac_header(skb2, 0);
-		dev_queue_xmit(skb2);
+		xmit_skb->protocol = __constant_htons(ETH_P_802_3);
+		skb_set_network_header(xmit_skb, 0);
+		skb_set_mac_header(xmit_skb, 0);
+		dev_queue_xmit(xmit_skb);
 	}
+}
+
+static ieee80211_txrx_result
+ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	u16 fc;
+	int err, hdrlen;
+
+	fc = rx->fc;
+	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
+		return TXRX_CONTINUE;
+
+	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
+		return TXRX_DROP;
+
+	hdrlen = ieee80211_get_hdrlen(fc);
+
+	if ((ieee80211_drop_802_1x_pae(rx, hdrlen)) ||
+	    (ieee80211_drop_unencrypted(rx, hdrlen)))
+		return TXRX_DROP;
+
+	err = ieee80211_data_to_8023(rx);
+	if (unlikely(err))
+		return TXRX_DROP;
+
+	rx->skb->dev = dev;
+
+	dev->stats.rx_packets++;
+	dev->stats.rx_bytes += rx->skb->len;
+
+	ieee80211_deliver_skb(rx);
 
 	return TXRX_QUEUED;
 }
@@ -1341,8 +1373,6 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
 	 * are not passed to user space by these functions
 	 */
 	ieee80211_rx_h_remove_qos_control,
-	ieee80211_rx_h_802_1x_pae,
-	ieee80211_rx_h_drop_unencrypted,
 	ieee80211_rx_h_data,
 	ieee80211_rx_h_mgmt,
 	NULL
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1a53154..8b3f056 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -420,7 +420,6 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_txrx_data *tx)
 	return TXRX_CONTINUE;
 }
 
-
 static ieee80211_txrx_result
 ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 {
@@ -433,13 +432,15 @@ ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 		return ieee80211_tx_h_multicast_ps_buf(tx);
 }
 
-
-
-
 static ieee80211_txrx_result
 ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 {
 	struct ieee80211_key *key;
+	const struct ieee80211_hdr *hdr;
+	u16 fc;
+
+	hdr = (const struct ieee80211_hdr *) tx->skb->data;
+	fc = le16_to_cpu(hdr->frame_control);
 
 	if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
 		tx->key = NULL;
@@ -448,7 +449,8 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 	else if ((key = rcu_dereference(tx->sdata->default_key)))
 		tx->key = key;
 	else if (tx->sdata->drop_unencrypted &&
-		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
+		 !(tx->sdata->eapol &&
+		   ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc)))) {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
 		return TXRX_DROP;
 	} else {
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 5a0564e..f90287a 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -217,23 +217,11 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
 
-int ieee80211_is_eapol(const struct sk_buff *skb)
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
 {
-	const struct ieee80211_hdr *hdr;
-	u16 fc;
-	int hdrlen;
-
 	if (unlikely(skb->len < 10))
 		return 0;
 
-	hdr = (const struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
-
-	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return 0;
-
-	hdrlen = ieee80211_get_hdrlen(fc);
-
 	if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
 		     memcmp(skb->data + hdrlen, eapol_header,
 			    sizeof(eapol_header)) == 0))
-- 
1.5.3.3
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 13018 bytes --]


On Thu, 2007-11-22 at 19:49 +0200, Ron Rindjunsky wrote:
> This patch restructures the Rx handlers chain by incorporating previously
> handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
> into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
> precisely after the IEEE802.11 data plane archituecture, and will prevent
> code duplication to IEEE8021.11n A-MSDU handler.
> 
> added function:
>  - ieee80211_data_to_8023: transfering 802.11 data frames to 802.3 frame
>  - ieee80211_deliver_skb: delivering the 802.3 frames to upper stack
> eliminated handlers:
>  - ieee80211_rx_h_drop_unencrypted: now function ieee80211_drop_unencrypted
>  - ieee80211_rx_h_802_1x_pae: now function ieee80211_802_1x_pae
> changed handlers:
>  - ieee80211_rx_h_data: now contains calls to four above function
> 
> Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>

Looks good to me, thanks.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

I'll clean out the eapol handling that looks a bit odd now w/o the
management interface in a later patch.

> ---
>  net/mac80211/ieee80211_i.h |    2 +-
>  net/mac80211/rx.c          |  124 +++++++++++++++++++++++++++-----------------
>  net/mac80211/tx.c          |   12 +++--
>  net/mac80211/util.c        |   14 +-----
>  4 files changed, 86 insertions(+), 66 deletions(-)
> 
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index b4e32ab..0444809 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -792,7 +792,7 @@ extern void *mac80211_wiphy_privid; /* for wiphy privid */
>  extern const unsigned char rfc1042_header[6];
>  extern const unsigned char bridge_tunnel_header[6];
>  u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
> -int ieee80211_is_eapol(const struct sk_buff *skb);
> +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
>  int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,
>  			     int rate, int erp, int short_preamble);
>  void mac80211_ev_michael_mic_failure(struct net_device *dev, int keyidx,
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 428a9fc..6333c98 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -956,68 +956,64 @@ ieee80211_rx_h_remove_qos_control(struct ieee80211_txrx_data *rx)
>  	return TXRX_CONTINUE;
>  }
>  
> -static ieee80211_txrx_result
> -ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
> +static int
> +ieee80211_drop_802_1x_pae(struct ieee80211_txrx_data *rx, int hdrlen)
>  {
> -	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
> +	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&
>  	    rx->sdata->type != IEEE80211_IF_TYPE_STA &&
>  	    (rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
> -		return TXRX_CONTINUE;
> +		return 0;
>  
>  	if (unlikely(rx->sdata->ieee802_1x &&
>  		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
>  		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
>  		     (!rx->sta || !(rx->sta->flags & WLAN_STA_AUTHORIZED)) &&
> -		     !ieee80211_is_eapol(rx->skb))) {
> +		     !ieee80211_is_eapol(rx->skb, hdrlen))) {
>  #ifdef CONFIG_MAC80211_DEBUG
> -		struct ieee80211_hdr *hdr =
> -			(struct ieee80211_hdr *) rx->skb->data;
> -		DECLARE_MAC_BUF(mac);
> -		printk(KERN_DEBUG "%s: dropped frame from %s"
> -		       " (unauthorized port)\n", rx->dev->name,
> -		       print_mac(mac, hdr->addr2));
> +		printk(KERN_DEBUG "%s: dropped frame "
> +		       "(unauthorized port)\n", rx->dev->name);
>  #endif /* CONFIG_MAC80211_DEBUG */
> -		return TXRX_DROP;
> +		return -EACCES;
>  	}
>  
> -	return TXRX_CONTINUE;
> +	return 0;
>  }
>  
> -static ieee80211_txrx_result
> -ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
> +static int
> +ieee80211_drop_unencrypted(struct ieee80211_txrx_data *rx, int hdrlen)
>  {
>  	/*
>  	 * Pass through unencrypted frames if the hardware has
>  	 * decrypted them already.
>  	 */
>  	if (rx->u.rx.status->flag & RX_FLAG_DECRYPTED)
> -		return TXRX_CONTINUE;
> +		return 0;
>  
>  	/* Drop unencrypted frames if key is set. */
>  	if (unlikely(!(rx->fc & IEEE80211_FCTL_PROTECTED) &&
>  		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
>  		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
>  		     rx->sdata->drop_unencrypted &&
> -		     (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
> +		     (!rx->sdata->eapol ||
> +		      !ieee80211_is_eapol(rx->skb, hdrlen)))) {
>  		if (net_ratelimit())
>  			printk(KERN_DEBUG "%s: RX non-WEP frame, but expected "
>  			       "encryption\n", rx->dev->name);
> -		return TXRX_DROP;
> +		return -EACCES;
>  	}
> -	return TXRX_CONTINUE;
> +	return 0;
>  }
>  
> -static ieee80211_txrx_result
> -ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
> +static int
> +ieee80211_data_to_8023(struct ieee80211_txrx_data *rx)
>  {
>  	struct net_device *dev = rx->dev;
> -	struct ieee80211_local *local = rx->local;
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
>  	u16 fc, hdrlen, ethertype;
>  	u8 *payload;
>  	u8 dst[ETH_ALEN];
>  	u8 src[ETH_ALEN];
> -	struct sk_buff *skb = rx->skb, *skb2;
> +	struct sk_buff *skb = rx->skb;
>  	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>  	DECLARE_MAC_BUF(mac);
>  	DECLARE_MAC_BUF(mac2);
> @@ -1025,11 +1021,9 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  	DECLARE_MAC_BUF(mac4);
>  
>  	fc = rx->fc;
> -	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
> -		return TXRX_CONTINUE;
>  
>  	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
> -		return TXRX_DROP;
> +		return -1;
>  
>  	hdrlen = ieee80211_get_hdrlen(fc);
>  
> @@ -1058,7 +1052,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  				       print_mac(mac, hdr->addr1),
>  				       print_mac(mac2, hdr->addr2),
>  				       print_mac(mac3, hdr->addr3));
> -			return TXRX_DROP;
> +			return -1;
>  		}
>  		break;
>  	case (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS):
> @@ -1075,7 +1069,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  				       print_mac(mac2, hdr->addr2),
>  				       print_mac(mac3, hdr->addr3),
>  				       print_mac(mac4, hdr->addr4));
> -			return TXRX_DROP;
> +			return -1;
>  		}
>  		break;
>  	case IEEE80211_FCTL_FROMDS:
> @@ -1086,7 +1080,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  		if (sdata->type != IEEE80211_IF_TYPE_STA ||
>  		    (is_multicast_ether_addr(dst) &&
>  		     !compare_ether_addr(src, dev->dev_addr)))
> -			return TXRX_DROP;
> +			return -1;
>  		break;
>  	case 0:
>  		/* DA SA BSSID */
> @@ -1102,21 +1096,20 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  				       print_mac(mac2, hdr->addr2),
>  				       print_mac(mac3, hdr->addr3));
>  			}
> -			return TXRX_DROP;
> +			return -1;
>  		}
>  		break;
>  	}
>  
> -	payload = skb->data + hdrlen;
> -
>  	if (unlikely(skb->len - hdrlen < 8)) {
>  		if (net_ratelimit()) {
>  			printk(KERN_DEBUG "%s: RX too short data frame "
>  			       "payload\n", dev->name);
>  		}
> -		return TXRX_DROP;
> +		return -1;
>  	}
>  
> +	payload = skb->data + hdrlen;
>  	ethertype = (payload[6] << 8) | payload[7];
>  
>  	if (likely((compare_ether_addr(payload, rfc1042_header) == 0 &&
> @@ -1137,12 +1130,19 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  		memcpy(ehdr->h_source, src, ETH_ALEN);
>  		ehdr->h_proto = len;
>  	}
> -	skb->dev = dev;
> +	return 0;
> +}
>  
> -	skb2 = NULL;
> +static void
> +ieee80211_deliver_skb(struct ieee80211_txrx_data *rx)
> +{
> +	struct net_device *dev = rx->dev;
> +	struct ieee80211_local *local = rx->local;
> +	struct sk_buff *skb, *xmit_skb;
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>  
> -	dev->stats.rx_packets++;
> -	dev->stats.rx_bytes += skb->len;
> +	skb = rx->skb;
> +	xmit_skb = NULL;
>  
>  	if (local->bridge_packets && (sdata->type == IEEE80211_IF_TYPE_AP
>  	    || sdata->type == IEEE80211_IF_TYPE_VLAN) &&
> @@ -1150,8 +1150,8 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  		if (is_multicast_ether_addr(skb->data)) {
>  			/* send multicast frames both to higher layers in
>  			 * local net stack and back to the wireless media */
> -			skb2 = skb_copy(skb, GFP_ATOMIC);
> -			if (!skb2 && net_ratelimit())
> +			xmit_skb = skb_copy(skb, GFP_ATOMIC);
> +			if (!xmit_skb && net_ratelimit())
>  				printk(KERN_DEBUG "%s: failed to clone "
>  				       "multicast frame\n", dev->name);
>  		} else {
> @@ -1166,7 +1166,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  				 * AP, so send the frame directly to it and
>  				 * do not pass the frame to local net stack.
>  				 */
> -				skb2 = skb;
> +				xmit_skb = skb;
>  				skb = NULL;
>  			}
>  			if (dsta)
> @@ -1181,13 +1181,45 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
>  		netif_rx(skb);
>  	}
>  
> -	if (skb2) {
> +	if (xmit_skb) {
>  		/* send to wireless media */
> -		skb2->protocol = __constant_htons(ETH_P_802_3);
> -		skb_set_network_header(skb2, 0);
> -		skb_set_mac_header(skb2, 0);
> -		dev_queue_xmit(skb2);
> +		xmit_skb->protocol = __constant_htons(ETH_P_802_3);
> +		skb_set_network_header(xmit_skb, 0);
> +		skb_set_mac_header(xmit_skb, 0);
> +		dev_queue_xmit(xmit_skb);
>  	}
> +}
> +
> +static ieee80211_txrx_result
> +ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
> +{
> +	struct net_device *dev = rx->dev;
> +	u16 fc;
> +	int err, hdrlen;
> +
> +	fc = rx->fc;
> +	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
> +		return TXRX_CONTINUE;
> +
> +	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
> +		return TXRX_DROP;
> +
> +	hdrlen = ieee80211_get_hdrlen(fc);
> +
> +	if ((ieee80211_drop_802_1x_pae(rx, hdrlen)) ||
> +	    (ieee80211_drop_unencrypted(rx, hdrlen)))
> +		return TXRX_DROP;
> +
> +	err = ieee80211_data_to_8023(rx);
> +	if (unlikely(err))
> +		return TXRX_DROP;
> +
> +	rx->skb->dev = dev;
> +
> +	dev->stats.rx_packets++;
> +	dev->stats.rx_bytes += rx->skb->len;
> +
> +	ieee80211_deliver_skb(rx);
>  
>  	return TXRX_QUEUED;
>  }
> @@ -1341,8 +1373,6 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
>  	 * are not passed to user space by these functions
>  	 */
>  	ieee80211_rx_h_remove_qos_control,
> -	ieee80211_rx_h_802_1x_pae,
> -	ieee80211_rx_h_drop_unencrypted,
>  	ieee80211_rx_h_data,
>  	ieee80211_rx_h_mgmt,
>  	NULL
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 1a53154..8b3f056 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -420,7 +420,6 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_txrx_data *tx)
>  	return TXRX_CONTINUE;
>  }
>  
> -
>  static ieee80211_txrx_result
>  ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
>  {
> @@ -433,13 +432,15 @@ ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
>  		return ieee80211_tx_h_multicast_ps_buf(tx);
>  }
>  
> -
> -
> -
>  static ieee80211_txrx_result
>  ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
>  {
>  	struct ieee80211_key *key;
> +	const struct ieee80211_hdr *hdr;
> +	u16 fc;
> +
> +	hdr = (const struct ieee80211_hdr *) tx->skb->data;
> +	fc = le16_to_cpu(hdr->frame_control);
>  
>  	if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
>  		tx->key = NULL;
> @@ -448,7 +449,8 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
>  	else if ((key = rcu_dereference(tx->sdata->default_key)))
>  		tx->key = key;
>  	else if (tx->sdata->drop_unencrypted &&
> -		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
> +		 !(tx->sdata->eapol &&
> +		   ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc)))) {
>  		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
>  		return TXRX_DROP;
>  	} else {
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 5a0564e..f90287a 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -217,23 +217,11 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
>  
> -int ieee80211_is_eapol(const struct sk_buff *skb)
> +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
>  {
> -	const struct ieee80211_hdr *hdr;
> -	u16 fc;
> -	int hdrlen;
> -
>  	if (unlikely(skb->len < 10))
>  		return 0;
>  
> -	hdr = (const struct ieee80211_hdr *) skb->data;
> -	fc = le16_to_cpu(hdr->frame_control);
> -
> -	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
> -		return 0;
> -
> -	hdrlen = ieee80211_get_hdrlen(fc);
> -
>  	if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
>  		     memcmp(skb->data + hdrlen, eapol_header,
>  			    sizeof(eapol_header)) == 0))

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

This patch restructures the Rx handlers chain by incorporating previously
handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
precisely after the IEEE802.11 data plane archituecture, and will prevent
code duplication to IEEE8021.11n A-MSDU handler.

added function:
 - ieee80211_data_to_8023: transfering 802.11 data frames to 802.3 frame
eliminated handlers:
 - ieee80211_rx_h_drop_unencrypted: now function ieee80211_drop_unencrypted
 - ieee80211_rx_h_802_1x_pae: now function ieee80211_802_1x_pae
changed handlers:
 - ieee80211_rx_h_data: now contains calls to three above function

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/ieee80211_i.h |    2 +-
 net/mac80211/rx.c          |   88 ++++++++++++++++++++++++++------------------
 net/mac80211/tx.c          |   12 +++--
 net/mac80211/util.c        |   14 +------
 4 files changed, 61 insertions(+), 55 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b4e32ab..0444809 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -792,7 +792,7 @@ extern void *mac80211_wiphy_privid; /* for wiphy privid */
 extern const unsigned char rfc1042_header[6];
 extern const unsigned char bridge_tunnel_header[6];
 u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
-int ieee80211_is_eapol(const struct sk_buff *skb);
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
 int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,
 			     int rate, int erp, int short_preamble);
 void mac80211_ev_michael_mic_failure(struct net_device *dev, int keyidx,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 428a9fc..497c27c 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -956,68 +956,64 @@ ieee80211_rx_h_remove_qos_control(struct ieee80211_txrx_data *rx)
 	return TXRX_CONTINUE;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_drop_802_1x_pae(struct ieee80211_txrx_data *rx, int hdrlen)
 {
-	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
+	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&
 	    rx->sdata->type != IEEE80211_IF_TYPE_STA &&
 	    (rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
-		return TXRX_CONTINUE;
+		return 0;
 
 	if (unlikely(rx->sdata->ieee802_1x &&
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     (!rx->sta || !(rx->sta->flags & WLAN_STA_AUTHORIZED)) &&
-		     !ieee80211_is_eapol(rx->skb))) {
+		     !ieee80211_is_eapol(rx->skb, hdrlen))) {
 #ifdef CONFIG_MAC80211_DEBUG
-		struct ieee80211_hdr *hdr =
-			(struct ieee80211_hdr *) rx->skb->data;
-		DECLARE_MAC_BUF(mac);
-		printk(KERN_DEBUG "%s: dropped frame from %s"
-		       " (unauthorized port)\n", rx->dev->name,
-		       print_mac(mac, hdr->addr2));
+		printk(KERN_DEBUG "%s: dropped frame "
+		       "(unauthorized port)\n", rx->dev->name);
 #endif /* CONFIG_MAC80211_DEBUG */
-		return TXRX_DROP;
+		return -EACCES;
 	}
 
-	return TXRX_CONTINUE;
+	return 0;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_drop_unencrypted(struct ieee80211_txrx_data *rx, int hdrlen)
 {
 	/*
 	 * Pass through unencrypted frames if the hardware has
 	 * decrypted them already.
 	 */
 	if (rx->u.rx.status->flag & RX_FLAG_DECRYPTED)
-		return TXRX_CONTINUE;
+		return 0;
 
 	/* Drop unencrypted frames if key is set. */
 	if (unlikely(!(rx->fc & IEEE80211_FCTL_PROTECTED) &&
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     rx->sdata->drop_unencrypted &&
-		     (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
+		     (!rx->sdata->eapol ||
+		      !ieee80211_is_eapol(rx->skb, hdrlen)))) {
 		if (net_ratelimit())
 			printk(KERN_DEBUG "%s: RX non-WEP frame, but expected "
 			       "encryption\n", rx->dev->name);
-		return TXRX_DROP;
+		return -EACCES;
 	}
-	return TXRX_CONTINUE;
+	return 0;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_data_to_8023(struct ieee80211_txrx_data *rx)
 {
 	struct net_device *dev = rx->dev;
-	struct ieee80211_local *local = rx->local;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
 	u16 fc, hdrlen, ethertype;
 	u8 *payload;
 	u8 dst[ETH_ALEN];
 	u8 src[ETH_ALEN];
-	struct sk_buff *skb = rx->skb, *skb2;
+	struct sk_buff *skb = rx->skb;
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	DECLARE_MAC_BUF(mac);
 	DECLARE_MAC_BUF(mac2);
@@ -1025,11 +1021,9 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 	DECLARE_MAC_BUF(mac4);
 
 	fc = rx->fc;
-	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
-		return TXRX_CONTINUE;
 
 	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return TXRX_DROP;
+		return -1;
 
 	hdrlen = ieee80211_get_hdrlen(fc);
 
@@ -1058,7 +1052,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac, hdr->addr1),
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3));
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	case (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS):
@@ -1075,7 +1069,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3),
 				       print_mac(mac4, hdr->addr4));
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	case IEEE80211_FCTL_FROMDS:
@@ -1086,7 +1080,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		if (sdata->type != IEEE80211_IF_TYPE_STA ||
 		    (is_multicast_ether_addr(dst) &&
 		     !compare_ether_addr(src, dev->dev_addr)))
-			return TXRX_DROP;
+			return -1;
 		break;
 	case 0:
 		/* DA SA BSSID */
@@ -1102,21 +1096,20 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3));
 			}
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	}
 
-	payload = skb->data + hdrlen;
-
 	if (unlikely(skb->len - hdrlen < 8)) {
 		if (net_ratelimit()) {
 			printk(KERN_DEBUG "%s: RX too short data frame "
 			       "payload\n", dev->name);
 		}
-		return TXRX_DROP;
+		return -1;
 	}
 
+	payload = skb->data + hdrlen;
 	ethertype = (payload[6] << 8) | payload[7];
 
 	if (likely((compare_ether_addr(payload, rfc1042_header) == 0 &&
@@ -1137,8 +1130,33 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		memcpy(ehdr->h_source, src, ETH_ALEN);
 		ehdr->h_proto = len;
 	}
-	skb->dev = dev;
+	return 0;
+}
 
+static ieee80211_txrx_result
+ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	struct ieee80211_local *local = rx->local;
+	u16 fc;
+	int err;
+	struct sk_buff *skb, *skb2;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+
+	fc = rx->fc;
+	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
+		return TXRX_CONTINUE;
+
+	err = ieee80211_data_to_8023(rx);
+	if (unlikely(err))
+		return TXRX_DROP;
+
+	if ((ieee80211_drop_802_1x_pae(rx, sizeof(struct ethhdr))) ||
+	    (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr))))
+		return TXRX_DROP;
+
+	skb = rx->skb;
+	skb->dev = dev;
 	skb2 = NULL;
 
 	dev->stats.rx_packets++;
@@ -1341,8 +1359,6 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
 	 * are not passed to user space by these functions
 	 */
 	ieee80211_rx_h_remove_qos_control,
-	ieee80211_rx_h_802_1x_pae,
-	ieee80211_rx_h_drop_unencrypted,
 	ieee80211_rx_h_data,
 	ieee80211_rx_h_mgmt,
 	NULL
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1a53154..8b3f056 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -420,7 +420,6 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_txrx_data *tx)
 	return TXRX_CONTINUE;
 }
 
-
 static ieee80211_txrx_result
 ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 {
@@ -433,13 +432,15 @@ ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 		return ieee80211_tx_h_multicast_ps_buf(tx);
 }
 
-
-
-
 static ieee80211_txrx_result
 ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 {
 	struct ieee80211_key *key;
+	const struct ieee80211_hdr *hdr;
+	u16 fc;
+
+	hdr = (const struct ieee80211_hdr *) tx->skb->data;
+	fc = le16_to_cpu(hdr->frame_control);
 
 	if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
 		tx->key = NULL;
@@ -448,7 +449,8 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 	else if ((key = rcu_dereference(tx->sdata->default_key)))
 		tx->key = key;
 	else if (tx->sdata->drop_unencrypted &&
-		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
+		 !(tx->sdata->eapol &&
+		   ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc)))) {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
 		return TXRX_DROP;
 	} else {
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 5a0564e..f90287a 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -217,23 +217,11 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
 
-int ieee80211_is_eapol(const struct sk_buff *skb)
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
 {
-	const struct ieee80211_hdr *hdr;
-	u16 fc;
-	int hdrlen;
-
 	if (unlikely(skb->len < 10))
 		return 0;
 
-	hdr = (const struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
-
-	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return 0;
-
-	hdrlen = ieee80211_get_hdrlen(fc);
-
 	if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
 		     memcmp(skb->data + hdrlen, eapol_header,
 			    sizeof(eapol_header)) == 0))
-- 
1.5.3.3
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

After seeing the corresponding a-msdu patch here are a few more
questions.

> +static ieee80211_txrx_result
> +ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
> +{
> +	struct net_device *dev = rx->dev;
> +	struct ieee80211_local *local = rx->local;
> +	u16 fc;
> +	int err;
> +	struct sk_buff *skb, *skb2;
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +
> +	fc = rx->fc;
> +	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
> +		return TXRX_CONTINUE;
> +
> +	err = ieee80211_data_to_8023(rx);
> +	if (unlikely(err))
> +		return TXRX_DROP;
> +
> +	if ((ieee80211_drop_802_1x_pae(rx, sizeof(struct ethhdr))) ||
> +	    (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr))))
> +		return TXRX_DROP;

I like this.

> +	skb = rx->skb;
> +	skb->dev = dev;
>  	skb2 = NULL;
>  
>  	dev->stats.rx_packets++;

Ok so the stuff that comes after this until the ned of the function is
responsible for delivering the packet. Maybe that should be split out
from the function too into something like ieee80211_deliver_skb()? In
the A-MSDU code you've had to basically copy all this code into the loop
as far as I can tell, something which I'd like to avoid very much.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

> > +static ieee80211_txrx_result
> > +ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
> > +{
> > +     struct net_device *dev = rx->dev;
> > +     struct ieee80211_local *local = rx->local;
> > +     u16 fc;
> > +     int err;
> > +     struct sk_buff *skb, *skb2;
> > +     struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> > +
> > +     fc = rx->fc;
> > +     if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
> > +             return TXRX_CONTINUE;
> > +
> > +     err = ieee80211_data_to_8023(rx);
> > +     if (unlikely(err))
> > +             return TXRX_DROP;
> > +
> > +     if ((ieee80211_drop_802_1x_pae(rx, sizeof(struct ethhdr))) ||
> > +         (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr))))
> > +             return TXRX_DROP;
>
> I like this.
>
> > +     skb = rx->skb;
> > +     skb->dev = dev;
> >       skb2 = NULL;
> >
> >       dev->stats.rx_packets++;
>
> Ok so the stuff that comes after this until the ned of the function is
> responsible for delivering the packet. Maybe that should be split out
> from the function too into something like ieee80211_deliver_skb()? In
> the A-MSDU code you've had to basically copy all this code into the loop
> as far as I can tell, something which I'd like to avoid very much.

I agree, it will be more readable that way. i'll do the needed changes.

>
> johannes
>
>

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

Hi Ron,

Sorry this will most likely in email crossing because I'm writing this
as I haven't been able to check mail since yesterday afternoon.

Looking into the eapol handling in more detail, I found another bug with
your patch. Namely, in the RX path, you have:

> -	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
> +	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&

at which point the frame is already reframed to 802.3.

However,

> -int ieee80211_is_eapol(const struct sk_buff *skb)
> +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
>  {

>  	if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
>  		     memcmp(skb->data + hdrlen, eapol_header,
>  			    sizeof(eapol_header)) == 0))

here checks the full 802.2 LLC header which was removed by the
reframing, thus this check will always be false.

I think we should, instead, have the ieee80211_data_to_8023 function set
skb->protocol correctly, and leave the skb's data pointer pointing to
the payload rather than the ethernet header, like eth_type_trans() would
do. In fact, I think we can just call it after we've reframed the frame.
Then, we can check skb->protocol for EAPOL which is a much cheaper check
too.

The thing I'm not entirely sure about is what happens with
eth_type_trans() and the pulling of the ethernet header. Wouldn't that
cause us problems with the skb->data[] accesses in
ieee80211_subif_start_xmit() once the frame arrives back there?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

> Looking into the eapol handling in more detail, I found another bug with
> your patch. Namely, in the RX path, you have:
>
> > -     if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
> > +     if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&
>
> at which point the frame is already reframed to 802.3.
>
> However,
>
> > -int ieee80211_is_eapol(const struct sk_buff *skb)
> > +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
> >  {
>
> >       if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
> >                    memcmp(skb->data + hdrlen, eapol_header,
> >                           sizeof(eapol_header)) == 0))
>
> here checks the full 802.2 LLC header which was removed by the
> reframing, thus this check will always be false.
>
> I think we should, instead, have the ieee80211_data_to_8023 function set
> skb->protocol correctly, and leave the skb's data pointer pointing to
> the payload rather than the ethernet header, like eth_type_trans() would
> do. In fact, I think we can just call it after we've reframed the frame.
> Then, we can check skb->protocol for EAPOL which is a much cheaper check
> too.
>
> The thing I'm not entirely sure about is what happens with
> eth_type_trans() and the pulling of the ethernet header. Wouldn't that
> cause us problems with the skb->data[] accesses in
> ieee80211_subif_start_xmit() once the frame arrives back there?

excellent catch, you are right.
let me suggest an alternative, which makes use of current ieee80211_is_eapol.
in regular Rx data frames, code will look like this:

+     ieee80211_drop_802_1x_pae(rx, size of current 802.11 hdr);
+     ieee80211_drop_unencrypted(rx, size of current 802.11 hdr);
      ieee80211_data_to_8023(rx)
-     ieee80211_drop_802_1x_pae(rx, size of 802.3 hdr);
-     ieee80211_drop_unencrypted(rx, size of 802.3 hdr);

while in A-MSDU frames code will look like this:

     ieee80211_data_to_8023(rx)
for each internal frame (that is any how in 802.2 form):
+     ieee80211_drop_802_1x_pae(rx, 0);
+     ieee80211_drop_unencrypted(rx, 0);
       move from 802.2 to 802.3
-     ieee80211_drop_802_1x_pae(rx, size of 802.3 hdr);
-     ieee80211_drop_unencrypted(rx, size of 802.3 hdr);

I thought it makes good use of the infrastructure we made so far, what
do you think?

> johannes
>
>

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]


> let me suggest an alternative, which makes use of current ieee80211_is_eapol.
> in regular Rx data frames, code will look like this:
> 
> +     ieee80211_drop_802_1x_pae(rx, size of current 802.11 hdr);
> +     ieee80211_drop_unencrypted(rx, size of current 802.11 hdr);
>       ieee80211_data_to_8023(rx)
> -     ieee80211_drop_802_1x_pae(rx, size of 802.3 hdr);
> -     ieee80211_drop_unencrypted(rx, size of 802.3 hdr);
> 
> while in A-MSDU frames code will look like this:
> 
>      ieee80211_data_to_8023(rx)
> for each internal frame (that is any how in 802.2 form):
> +     ieee80211_drop_802_1x_pae(rx, 0);
> +     ieee80211_drop_unencrypted(rx, 0);
>        move from 802.2 to 802.3
> -     ieee80211_drop_802_1x_pae(rx, size of 802.3 hdr);
> -     ieee80211_drop_unencrypted(rx, size of 802.3 hdr);
> 
> I thought it makes good use of the infrastructure we made so far, what
> do you think?

Yes, that would work too. I'm not entirely sure about the effect on
eapol handling but I'll sort that out with that patch then. At worst I
get to restructure the code instead of you :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]


> Yes, that would work too. I'm not entirely sure about the effect on
> eapol handling but I'll sort that out with that patch then. At worst I
> get to restructure the code instead of you :)

Actually, what I did in that patch is move the eapol decision out of the
port control function which is probably better. Anyway, if you can do
the restructuring and make it work as it is now, I'll do the rest.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

This patch restructures the Rx handlers chain by incorporating previously
handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
precisely after the IEEE802.11 data plane archituecture, and will prevent
code duplication to IEEE8021.11n A-MSDU handler.

added function:
 - ieee80211_data_to_8023: transfering 802.11 data frames to 802.3 frame
eliminated handlers:
 - ieee80211_rx_h_drop_unencrypted: now function ieee80211_drop_unencrypted
 - ieee80211_rx_h_802_1x_pae: now function ieee80211_802_1x_pae
changed handlers:
 - ieee80211_rx_h_data: now contains calls to three above function

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/ieee80211_i.h |    2 +-
 net/mac80211/rx.c          |   88 ++++++++++++++++++++++++++------------------
 net/mac80211/tx.c          |   12 +++--
 net/mac80211/util.c        |   14 +------
 4 files changed, 61 insertions(+), 55 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b4e32ab..0444809 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -792,7 +792,7 @@ extern void *mac80211_wiphy_privid; /* for wiphy privid */
 extern const unsigned char rfc1042_header[6];
 extern const unsigned char bridge_tunnel_header[6];
 u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
-int ieee80211_is_eapol(const struct sk_buff *skb);
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
 int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,
 			     int rate, int erp, int short_preamble);
 void mac80211_ev_michael_mic_failure(struct net_device *dev, int keyidx,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 428a9fc..59a260e 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -956,68 +956,64 @@ ieee80211_rx_h_remove_qos_control(struct ieee80211_txrx_data *rx)
 	return TXRX_CONTINUE;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_drop_802_1x_pae(struct ieee80211_txrx_data *rx, int hdrlen)
 {
-	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
+	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&
 	    rx->sdata->type != IEEE80211_IF_TYPE_STA &&
 	    (rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
-		return TXRX_CONTINUE;
+		return 0;
 
 	if (unlikely(rx->sdata->ieee802_1x &&
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     (!rx->sta || !(rx->sta->flags & WLAN_STA_AUTHORIZED)) &&
-		     !ieee80211_is_eapol(rx->skb))) {
+		     !ieee80211_is_eapol(rx->skb, hdrlen))) {
 #ifdef CONFIG_MAC80211_DEBUG
-		struct ieee80211_hdr *hdr =
-			(struct ieee80211_hdr *) rx->skb->data;
-		DECLARE_MAC_BUF(mac);
-		printk(KERN_DEBUG "%s: dropped frame from %s"
-		       " (unauthorized port)\n", rx->dev->name,
-		       print_mac(mac, hdr->addr2));
+		printk(KERN_DEBUG "%s: dropped frame "
+		       "(unauthorized port)\n", rx->dev->name);
 #endif /* CONFIG_MAC80211_DEBUG */
-		return TXRX_DROP;
+		return -EACCES;
 	}
 
-	return TXRX_CONTINUE;
+	return 0;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_drop_unencrypted(struct ieee80211_txrx_data *rx, int hdrlen)
 {
 	/*
 	 * Pass through unencrypted frames if the hardware has
 	 * decrypted them already.
 	 */
 	if (rx->u.rx.status->flag & RX_FLAG_DECRYPTED)
-		return TXRX_CONTINUE;
+		return 0;
 
 	/* Drop unencrypted frames if key is set. */
 	if (unlikely(!(rx->fc & IEEE80211_FCTL_PROTECTED) &&
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     rx->sdata->drop_unencrypted &&
-		     (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
+		     (rx->sdata->eapol ||
+		      !ieee80211_is_eapol(rx->skb, hdrlen)))) {
 		if (net_ratelimit())
 			printk(KERN_DEBUG "%s: RX non-WEP frame, but expected "
 			       "encryption\n", rx->dev->name);
-		return TXRX_DROP;
+		return -EACCES;
 	}
-	return TXRX_CONTINUE;
+	return 0;
 }
 
-static ieee80211_txrx_result
-ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+static int
+ieee80211_data_to_8023(struct ieee80211_txrx_data *rx)
 {
 	struct net_device *dev = rx->dev;
-	struct ieee80211_local *local = rx->local;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
 	u16 fc, hdrlen, ethertype;
 	u8 *payload;
 	u8 dst[ETH_ALEN];
 	u8 src[ETH_ALEN];
-	struct sk_buff *skb = rx->skb, *skb2;
+	struct sk_buff *skb = rx->skb;
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	DECLARE_MAC_BUF(mac);
 	DECLARE_MAC_BUF(mac2);
@@ -1025,11 +1021,9 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 	DECLARE_MAC_BUF(mac4);
 
 	fc = rx->fc;
-	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
-		return TXRX_CONTINUE;
 
 	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return TXRX_DROP;
+		return -1;
 
 	hdrlen = ieee80211_get_hdrlen(fc);
 
@@ -1058,7 +1052,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac, hdr->addr1),
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3));
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	case (IEEE80211_FCTL_TODS | IEEE80211_FCTL_FROMDS):
@@ -1075,7 +1069,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3),
 				       print_mac(mac4, hdr->addr4));
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	case IEEE80211_FCTL_FROMDS:
@@ -1086,7 +1080,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		if (sdata->type != IEEE80211_IF_TYPE_STA ||
 		    (is_multicast_ether_addr(dst) &&
 		     !compare_ether_addr(src, dev->dev_addr)))
-			return TXRX_DROP;
+			return -1;
 		break;
 	case 0:
 		/* DA SA BSSID */
@@ -1102,21 +1096,20 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 				       print_mac(mac2, hdr->addr2),
 				       print_mac(mac3, hdr->addr3));
 			}
-			return TXRX_DROP;
+			return -1;
 		}
 		break;
 	}
 
-	payload = skb->data + hdrlen;
-
 	if (unlikely(skb->len - hdrlen < 8)) {
 		if (net_ratelimit()) {
 			printk(KERN_DEBUG "%s: RX too short data frame "
 			       "payload\n", dev->name);
 		}
-		return TXRX_DROP;
+		return -1;
 	}
 
+	payload = skb->data + hdrlen;
 	ethertype = (payload[6] << 8) | payload[7];
 
 	if (likely((compare_ether_addr(payload, rfc1042_header) == 0 &&
@@ -1137,8 +1130,33 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		memcpy(ehdr->h_source, src, ETH_ALEN);
 		ehdr->h_proto = len;
 	}
-	skb->dev = dev;
+	return 0;
+}
 
+static ieee80211_txrx_result
+ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	struct ieee80211_local *local = rx->local;
+	u16 fc;
+	int err;
+	struct sk_buff *skb, *skb2;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+
+	fc = rx->fc;
+	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
+		return TXRX_CONTINUE;
+
+	err = ieee80211_data_to_8023(rx);
+	if (unlikely(err))
+		return TXRX_DROP;
+
+	if ((ieee80211_drop_802_1x_pae(rx, sizeof(struct ethhdr))) ||
+	    (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr))))
+		return TXRX_DROP;
+
+	skb = rx->skb;
+	skb->dev = dev;
 	skb2 = NULL;
 
 	dev->stats.rx_packets++;
@@ -1341,8 +1359,6 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
 	 * are not passed to user space by these functions
 	 */
 	ieee80211_rx_h_remove_qos_control,
-	ieee80211_rx_h_802_1x_pae,
-	ieee80211_rx_h_drop_unencrypted,
 	ieee80211_rx_h_data,
 	ieee80211_rx_h_mgmt,
 	NULL
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1a53154..8b3f056 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -420,7 +420,6 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_txrx_data *tx)
 	return TXRX_CONTINUE;
 }
 
-
 static ieee80211_txrx_result
 ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 {
@@ -433,13 +432,15 @@ ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 		return ieee80211_tx_h_multicast_ps_buf(tx);
 }
 
-
-
-
 static ieee80211_txrx_result
 ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 {
 	struct ieee80211_key *key;
+	const struct ieee80211_hdr *hdr;
+	u16 fc;
+
+	hdr = (const struct ieee80211_hdr *) tx->skb->data;
+	fc = le16_to_cpu(hdr->frame_control);
 
 	if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
 		tx->key = NULL;
@@ -448,7 +449,8 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 	else if ((key = rcu_dereference(tx->sdata->default_key)))
 		tx->key = key;
 	else if (tx->sdata->drop_unencrypted &&
-		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
+		 !(tx->sdata->eapol &&
+		   ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc)))) {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
 		return TXRX_DROP;
 	} else {
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 5a0564e..f90287a 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -217,23 +217,11 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
 
-int ieee80211_is_eapol(const struct sk_buff *skb)
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
 {
-	const struct ieee80211_hdr *hdr;
-	u16 fc;
-	int hdrlen;
-
 	if (unlikely(skb->len < 10))
 		return 0;
 
-	hdr = (const struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
-
-	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return 0;
-
-	hdrlen = ieee80211_get_hdrlen(fc);
-
 	if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
 		     memcmp(skb->data + hdrlen, eapol_header,
 			    sizeof(eapol_header)) == 0))
-- 
1.5.3.3
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

On 11/20/07, Ron Rindjunsky <ron.rindjunsky@intel.com> wrote:
> This patch restructures the Rx handlers chain by incorporating previously
> handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
> into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
> precisely after the IEEE802.11 data plane archituecture, and will prevent
> code duplication to IEEE8021.11n A-MSDU handler.

Sorry for the slip, I will issue the *correct* patch :)

> -                    (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
> +                    (rx->sdata->eapol ||

[PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

This patch restructures the Rx handlers chain by incorporating previously
handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
precisely after the IEEE802.11 data plane archituecture, and will prevent
code duplication to IEEE8021.11n A-MSDU handler.

added function:
 - ieee80211_data_to_8023: transfering 802.11 data frames to 802.3 frame
eliminated handlers:
 - ieee80211_rx_h_drop_unencrypted: now function ieee80211_drop_unencrypted
 - ieee80211_rx_h_802_1x_pae: now function ieee80211_802_1x_pae
changed handlers:
 - ieee80211_rx_h_data: now contains calls to three above function

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
 net/mac80211/ieee80211_i.h |    2 +-
 net/mac80211/rx.c          |   56 +++++++++++++++++++++++++++----------------
 net/mac80211/tx.c          |   13 ++++++----
 net/mac80211/util.c        |   14 +----------
 4 files changed, 45 insertions(+), 40 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b4e32ab..0444809 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -792,7 +792,7 @@ extern void *mac80211_wiphy_privid; /* for wiphy privid */
 extern const unsigned char rfc1042_header[6];
 extern const unsigned char bridge_tunnel_header[6];
 u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len);
-int ieee80211_is_eapol(const struct sk_buff *skb);
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
 int ieee80211_frame_duration(struct ieee80211_local *local, size_t len,
 			     int rate, int erp, int short_preamble);
 void mac80211_ev_michael_mic_failure(struct net_device *dev, int keyidx,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 428a9fc..1909662 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -957,9 +957,9 @@ ieee80211_rx_h_remove_qos_control(struct ieee80211_txrx_data *rx)
 }
 
 static ieee80211_txrx_result
-ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
+ieee80211_802_1x_pae(struct ieee80211_txrx_data *rx, int hdrlen)
 {
-	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb) &&
+	if (rx->sdata->eapol && ieee80211_is_eapol(rx->skb, hdrlen) &&
 	    rx->sdata->type != IEEE80211_IF_TYPE_STA &&
 	    (rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
 		return TXRX_CONTINUE;
@@ -968,14 +968,10 @@ ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     (!rx->sta || !(rx->sta->flags & WLAN_STA_AUTHORIZED)) &&
-		     !ieee80211_is_eapol(rx->skb))) {
+		     !ieee80211_is_eapol(rx->skb, hdrlen))) {
 #ifdef CONFIG_MAC80211_DEBUG
-		struct ieee80211_hdr *hdr =
-			(struct ieee80211_hdr *) rx->skb->data;
-		DECLARE_MAC_BUF(mac);
-		printk(KERN_DEBUG "%s: dropped frame from %s"
-		       " (unauthorized port)\n", rx->dev->name,
-		       print_mac(mac, hdr->addr2));
+		printk(KERN_DEBUG "%s: dropped frame "
+		       "(unauthorized port)\n", rx->dev->name);
 #endif /* CONFIG_MAC80211_DEBUG */
 		return TXRX_DROP;
 	}
@@ -984,7 +980,7 @@ ieee80211_rx_h_802_1x_pae(struct ieee80211_txrx_data *rx)
 }
 
 static ieee80211_txrx_result
-ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
+ieee80211_drop_unencrypted(struct ieee80211_txrx_data *rx, int hdrlen)
 {
 	/*
 	 * Pass through unencrypted frames if the hardware has
@@ -998,7 +994,8 @@ ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
 		     (rx->fc & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA &&
 		     (rx->fc & IEEE80211_FCTL_STYPE) != IEEE80211_STYPE_NULLFUNC &&
 		     rx->sdata->drop_unencrypted &&
-		     (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
+		     (rx->sdata->eapol == 0 ||
+		      !ieee80211_is_eapol(rx->skb, hdrlen)))) {
 		if (net_ratelimit())
 			printk(KERN_DEBUG "%s: RX non-WEP frame, but expected "
 			       "encryption\n", rx->dev->name);
@@ -1008,16 +1005,15 @@ ieee80211_rx_h_drop_unencrypted(struct ieee80211_txrx_data *rx)
 }
 
 static ieee80211_txrx_result
-ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+ieee80211_data_to_8023(struct ieee80211_txrx_data *rx)
 {
 	struct net_device *dev = rx->dev;
-	struct ieee80211_local *local = rx->local;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
 	u16 fc, hdrlen, ethertype;
 	u8 *payload;
 	u8 dst[ETH_ALEN];
 	u8 src[ETH_ALEN];
-	struct sk_buff *skb = rx->skb, *skb2;
+	struct sk_buff *skb = rx->skb;
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	DECLARE_MAC_BUF(mac);
 	DECLARE_MAC_BUF(mac2);
@@ -1025,8 +1021,6 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 	DECLARE_MAC_BUF(mac4);
 
 	fc = rx->fc;
-	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
-		return TXRX_CONTINUE;
 
 	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
 		return TXRX_DROP;
@@ -1107,8 +1101,6 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		break;
 	}
 
-	payload = skb->data + hdrlen;
-
 	if (unlikely(skb->len - hdrlen < 8)) {
 		if (net_ratelimit()) {
 			printk(KERN_DEBUG "%s: RX too short data frame "
@@ -1117,6 +1109,7 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		return TXRX_DROP;
 	}
 
+	payload = skb->data + hdrlen;
 	ethertype = (payload[6] << 8) | payload[7];
 
 	if (likely((compare_ether_addr(payload, rfc1042_header) == 0 &&
@@ -1137,8 +1130,31 @@ ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
 		memcpy(ehdr->h_source, src, ETH_ALEN);
 		ehdr->h_proto = len;
 	}
-	skb->dev = dev;
+	return TXRX_CONTINUE;
+}
+
+static ieee80211_txrx_result
+ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
+{
+	struct net_device *dev = rx->dev;
+	struct ieee80211_local *local = rx->local;
+	u16 fc;
+	struct sk_buff *skb, *skb2;
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
+	fc = rx->fc;
+	if (unlikely((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA))
+		return TXRX_CONTINUE;
+
+	if (ieee80211_data_to_8023(rx) == TXRX_DROP)
+		return TXRX_DROP;
+	if (ieee80211_802_1x_pae(rx, sizeof(struct ethhdr)) == TXRX_DROP)
+		return TXRX_DROP;
+	if (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr)) == TXRX_DROP)
+		return TXRX_DROP;
+
+	skb = rx->skb;
+	skb->dev = dev;
 	skb2 = NULL;
 
 	dev->stats.rx_packets++;
@@ -1341,8 +1357,6 @@ ieee80211_rx_handler ieee80211_rx_handlers[] =
 	 * are not passed to user space by these functions
 	 */
 	ieee80211_rx_h_remove_qos_control,
-	ieee80211_rx_h_802_1x_pae,
-	ieee80211_rx_h_drop_unencrypted,
 	ieee80211_rx_h_data,
 	ieee80211_rx_h_mgmt,
 	NULL
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1a53154..9a2553f 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -420,7 +420,6 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_txrx_data *tx)
 	return TXRX_CONTINUE;
 }
 
-
 static ieee80211_txrx_result
 ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 {
@@ -433,13 +432,17 @@ ieee80211_tx_h_ps_buf(struct ieee80211_txrx_data *tx)
 		return ieee80211_tx_h_multicast_ps_buf(tx);
 }
 
-
-
-
 static ieee80211_txrx_result
 ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 {
 	struct ieee80211_key *key;
+	const struct ieee80211_hdr *hdr;
+	u16 fc;
+	int hdrlen;
+
+	hdr = (const struct ieee80211_hdr *) tx->skb->data;
+	fc = le16_to_cpu(hdr->frame_control);
+	hdrlen = ieee80211_get_hdrlen(fc);
 
 	if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
 		tx->key = NULL;
@@ -448,7 +451,7 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
 	else if ((key = rcu_dereference(tx->sdata->default_key)))
 		tx->key = key;
 	else if (tx->sdata->drop_unencrypted &&
-		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
+		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb, hdrlen))) {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
 		return TXRX_DROP;
 	} else {
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 5a0564e..f90287a 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -217,23 +217,11 @@ int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(ieee80211_get_hdrlen_from_skb);
 
-int ieee80211_is_eapol(const struct sk_buff *skb)
+int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen)
 {
-	const struct ieee80211_hdr *hdr;
-	u16 fc;
-	int hdrlen;
-
 	if (unlikely(skb->len < 10))
 		return 0;
 
-	hdr = (const struct ieee80211_hdr *) skb->data;
-	fc = le16_to_cpu(hdr->frame_control);
-
-	if (unlikely(!WLAN_FC_DATA_PRESENT(fc)))
-		return 0;
-
-	hdrlen = ieee80211_get_hdrlen(fc);
-
 	if (unlikely(skb->len >= hdrlen + sizeof(eapol_header) &&
 		     memcmp(skb->data + hdrlen, eapol_header,
 			    sizeof(eapol_header)) == 0))
-- 
1.5.3.3
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]


On Mon, 2007-11-19 at 15:34 +0200, Ron Rindjunsky wrote:
> This patch restructures the Rx handlers chain by incorporating previously
> handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
> into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
> precisely after the IEEE802.11 data plane archituecture, and will prevent
> code duplication to IEEE8021.11n A-MSDU handler.

Nice, thanks for doing this. Few comments and questions below.


> -int ieee80211_is_eapol(const struct sk_buff *skb);
> +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);

That's just an optimisation, right?

>  #ifdef CONFIG_MAC80211_DEBUG
> -		struct ieee80211_hdr *hdr =
> -			(struct ieee80211_hdr *) rx->skb->data;
> -		DECLARE_MAC_BUF(mac);
> -		printk(KERN_DEBUG "%s: dropped frame from %s"
> -		       " (unauthorized port)\n", rx->dev->name,
> -		       print_mac(mac, hdr->addr2));
> +		printk(KERN_DEBUG "%s: dropped frame "
> +		       "(unauthorized port)\n", rx->dev->name);

Any reason you're getting rid of the MAC printk? It's a debug-only
message so whatever, just curious really.

> -		     (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
> +		     (rx->sdata->eapol == 0 ||
> +		      !ieee80211_is_eapol(rx->skb, hdrlen)))) {

I guess you could make it look nicer by doing !rx->sdata->eapol || ...

> +	if (ieee80211_data_to_8023(rx) == TXRX_DROP)
> +		return TXRX_DROP;
> +	if (ieee80211_802_1x_pae(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> +		return TXRX_DROP;
> +	if (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> +		return TXRX_DROP;

The sizeof() doesn't look right, shouldn't that get the hdrlen from the
frame control? Ah, no, I see now, it's already converted at this point.
Do we need to pass the hdrlen at all then? I think it would make sense
to specify (in comments) that those functions get a converted (to 802.3
framing) skb instead. And then hardcode the frame offsets/header length.

Also, I think the return value of those functions shouldn't be the
txrx_result but rather just a bool.

>  static ieee80211_txrx_result
>  ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
>  {
>  	struct ieee80211_key *key;
> +	const struct ieee80211_hdr *hdr;
> +	u16 fc;
> +	int hdrlen;
> +
> +	hdr = (const struct ieee80211_hdr *) tx->skb->data;
> +	fc = le16_to_cpu(hdr->frame_control);
> +	hdrlen = ieee80211_get_hdrlen(fc);
>  
>  	if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
>  		tx->key = NULL;
> @@ -448,7 +451,7 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
>  	else if ((key = rcu_dereference(tx->sdata->default_key)))
>  		tx->key = key;
>  	else if (tx->sdata->drop_unencrypted &&
> -		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
> +		 !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb, hdrlen))) {

I think it would make sense to have another function
ieee80211_is_eapol_checked(tx->skb) that does the hdrlen calculation to
save doing it unconditionally here for the (common) case where we're
using a sta or default key.

Overall, nice patch.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

> > This patch restructures the Rx handlers chain by incorporating previously
> > handlers ieee80211_rx_h_802_1x_pae and ieee80211_rx_h_drop_unencrypted
> > into ieee80211_rx_h_data, already in 802.3 form. this scheme follows more
> > precisely after the IEEE802.11 data plane archituecture, and will prevent
> > code duplication to IEEE8021.11n A-MSDU handler.
>
> Nice, thanks for doing this. Few comments and questions below.
>
>
> > -int ieee80211_is_eapol(const struct sk_buff *skb);
> > +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
>
> That's just an optimisation, right?

main reason is that ieee80211_is_eapol is called both in Tx and Rx
flows, so the caller is reaponsible for passing the header length
(802.3 in Rx and 802.11 in Tx).

>
> >  #ifdef CONFIG_MAC80211_DEBUG
> > -             struct ieee80211_hdr *hdr =
> > -                     (struct ieee80211_hdr *) rx->skb->data;
> > -             DECLARE_MAC_BUF(mac);
> > -             printk(KERN_DEBUG "%s: dropped frame from %s"
> > -                    " (unauthorized port)\n", rx->dev->name,
> > -                    print_mac(mac, hdr->addr2));
> > +             printk(KERN_DEBUG "%s: dropped frame "
> > +                    "(unauthorized port)\n", rx->dev->name);
>
> Any reason you're getting rid of the MAC printk? It's a debug-only
> message so whatever, just curious really.
>

as ieee80211_802_1x_pae is not a 802.11 specific any more, it has no knowledge
about the header, therefore no knowledge about mac address. I could
pass to it the mac address as well, but looks odd to do that only for
debug printing.

> > -                  (rx->sdata->eapol == 0 || !ieee80211_is_eapol(rx->skb)))) {
> > +                  (rx->sdata->eapol == 0 ||
> > +                   !ieee80211_is_eapol(rx->skb, hdrlen)))) {
>
> I guess you could make it look nicer by doing !rx->sdata->eapol || ...

no problems, will do

>
> > +     if (ieee80211_data_to_8023(rx) == TXRX_DROP)
> > +             return TXRX_DROP;
> > +     if (ieee80211_802_1x_pae(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> > +             return TXRX_DROP;
> > +     if (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> > +             return TXRX_DROP;
>
> The sizeof() doesn't look right, shouldn't that get the hdrlen from the
> frame control? Ah, no, I see now, it's already converted at this point.
> Do we need to pass the hdrlen at all then? I think it would make sense
> to specify (in comments) that those functions get a converted (to 802.3
> framing) skb instead. And then hardcode the frame offsets/header length.

those functions can be used now for 802.11 or 802.3 frames so caller
should pass header length for them

>
> Also, I think the return value of those functions shouldn't be the
> txrx_result but rather just a bool.
>

Hmmm... yes, it will probebly look better. i'll just do a slight
change in function's names so it will be clearer to reader.

> >  static ieee80211_txrx_result
> >  ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
> >  {
> >       struct ieee80211_key *key;
> > +     const struct ieee80211_hdr *hdr;
> > +     u16 fc;
> > +     int hdrlen;
> > +
> > +     hdr = (const struct ieee80211_hdr *) tx->skb->data;
> > +     fc = le16_to_cpu(hdr->frame_control);
> > +     hdrlen = ieee80211_get_hdrlen(fc);
> >
> >       if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
> >               tx->key = NULL;
> > @@ -448,7 +451,7 @@ ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
> >       else if ((key = rcu_dereference(tx->sdata->default_key)))
> >               tx->key = key;
> >       else if (tx->sdata->drop_unencrypted &&
> > -              !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
> > +              !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb, hdrlen))) {
>
> I think it would make sense to have another function
> ieee80211_is_eapol_checked(tx->skb) that does the hdrlen calculation to
> save doing it unconditionally here for the (common) case where we're
> using a sta or default key.

I agree, thanks for the remark.
Yet, i wouldn't like to add another function that does basically the
same as ieee80211_is_eapol (or sending an indication for
ieee80211_is_eapol about header type what will lessen its
flexsibility)
I was thinking to move ieee80211_get_hdrlen to:
ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc))
this way i keep flexibility in ieee80211_is_eapol and won't calculate
header length unless really needed.

>
> Overall, nice patch.
>
> johannes
>
>

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]


> > > -int ieee80211_is_eapol(const struct sk_buff *skb);
> > > +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
> >
> > That's just an optimisation, right?
> 
> main reason is that ieee80211_is_eapol is called both in Tx and Rx
> flows, so the caller is reaponsible for passing the header length
> (802.3 in Rx and 802.11 in Tx).

Right. I noticed later but forgot to remove the comment.

> > Any reason you're getting rid of the MAC printk? It's a debug-only
> > message so whatever, just curious really.
> >
> 
> as ieee80211_802_1x_pae is not a 802.11 specific any more, it has no knowledge
> about the header, therefore no knowledge about mac address. I could
> pass to it the mac address as well, but looks odd to do that only for
> debug printing.

Oh ok, right.

> > > +     if (ieee80211_data_to_8023(rx) == TXRX_DROP)
> > > +             return TXRX_DROP;
> > > +     if (ieee80211_802_1x_pae(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> > > +             return TXRX_DROP;
> > > +     if (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> > > +             return TXRX_DROP;
> >
> > The sizeof() doesn't look right, shouldn't that get the hdrlen from the
> > frame control? Ah, no, I see now, it's already converted at this point.
> > Do we need to pass the hdrlen at all then? I think it would make sense
> > to specify (in comments) that those functions get a converted (to 802.3
> > framing) skb instead. And then hardcode the frame offsets/header length.
> 
> those functions can be used now for 802.11 or 802.3 frames so caller
> should pass header length for them
> 
> >
> > Also, I think the return value of those functions shouldn't be the
> > txrx_result but rather just a bool.
> >
> 
> Hmmm... yes, it will probebly look better. i'll just do a slight
> change in function's names so it will be clearer to reader.

Great, thanks.

> > I think it would make sense to have another function
> > ieee80211_is_eapol_checked(tx->skb) that does the hdrlen calculation to
> > save doing it unconditionally here for the (common) case where we're
> > using a sta or default key.
> 
> I agree, thanks for the remark.
> Yet, i wouldn't like to add another function that does basically the
> same as ieee80211_is_eapol (or sending an indication for
> ieee80211_is_eapol about header type what will lessen its
> flexsibility)
> I was thinking to move ieee80211_get_hdrlen to:
> ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc))
> this way i keep flexibility in ieee80211_is_eapol and won't calculate
> header length unless really needed.

Oh sure, that works too. I guess you could inline all of it even
ieee80211_is_eapol(tx->skb,
                   ieee80211_get_hdrlen(le16_to_cpu(hdr->frame_control)))

But yeah, whatever, I'm not concerned about the byteswap.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: restructuring data Rx handlers

[Ron Rindjunsky]

Great, so needed changes are on their way, and i will issue tomorrow
the A-MSDU patch that uses the new handlers scheme.
thanks
ron

> > > > -int ieee80211_is_eapol(const struct sk_buff *skb);
> > > > +int ieee80211_is_eapol(const struct sk_buff *skb, int hdrlen);
> > >
> > > That's just an optimisation, right?
> >
> > main reason is that ieee80211_is_eapol is called both in Tx and Rx
> > flows, so the caller is reaponsible for passing the header length
> > (802.3 in Rx and 802.11 in Tx).
>
> Right. I noticed later but forgot to remove the comment.
>
> > > Any reason you're getting rid of the MAC printk? It's a debug-only
> > > message so whatever, just curious really.
> > >
> >
> > as ieee80211_802_1x_pae is not a 802.11 specific any more, it has no knowledge
> > about the header, therefore no knowledge about mac address. I could
> > pass to it the mac address as well, but looks odd to do that only for
> > debug printing.
>
> Oh ok, right.
>
> > > > +     if (ieee80211_data_to_8023(rx) == TXRX_DROP)
> > > > +             return TXRX_DROP;
> > > > +     if (ieee80211_802_1x_pae(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> > > > +             return TXRX_DROP;
> > > > +     if (ieee80211_drop_unencrypted(rx, sizeof(struct ethhdr)) == TXRX_DROP)
> > > > +             return TXRX_DROP;
> > >
> > > The sizeof() doesn't look right, shouldn't that get the hdrlen from the
> > > frame control? Ah, no, I see now, it's already converted at this point.
> > > Do we need to pass the hdrlen at all then? I think it would make sense
> > > to specify (in comments) that those functions get a converted (to 802.3
> > > framing) skb instead. And then hardcode the frame offsets/header length.
> >
> > those functions can be used now for 802.11 or 802.3 frames so caller
> > should pass header length for them
> >
> > >
> > > Also, I think the return value of those functions shouldn't be the
> > > txrx_result but rather just a bool.
> > >
> >
> > Hmmm... yes, it will probebly look better. i'll just do a slight
> > change in function's names so it will be clearer to reader.
>
> Great, thanks.
>
> > > I think it would make sense to have another function
> > > ieee80211_is_eapol_checked(tx->skb) that does the hdrlen calculation to
> > > save doing it unconditionally here for the (common) case where we're
> > > using a sta or default key.
> >
> > I agree, thanks for the remark.
> > Yet, i wouldn't like to add another function that does basically the
> > same as ieee80211_is_eapol (or sending an indication for
> > ieee80211_is_eapol about header type what will lessen its
> > flexsibility)
> > I was thinking to move ieee80211_get_hdrlen to:
> > ieee80211_is_eapol(tx->skb, ieee80211_get_hdrlen(fc))
> > this way i keep flexibility in ieee80211_is_eapol and won't calculate
> > header length unless really needed.
>
> Oh sure, that works too. I guess you could inline all of it even
> ieee80211_is_eapol(tx->skb,
>                   ieee80211_get_hdrlen(le16_to_cpu(hdr->frame_control)))
>
> But yeah, whatever, I'm not concerned about the byteswap.
>
> johannes
>
>