> > 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
>
>
-
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
