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:
signature.asc
Description: This is a digitally signed message part
