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