On Mon, 2007-03-26 at 04:40 -0700, mohamed wrote:
Scratch my previous comments. Sorry about that, I should've looked
better.
> +inline static int is_agg_frame(struct sk_buff *skb, u16 fc)
> +{
> + u8 *p = skb->data + ieee80211_get_hdrlen(fc) - 2;
See below, but isn't this broken if somebody includes an HT field?
> + if (!(fc & IEEE80211_STYPE_QOS_DATA));
> + return 0;
> +
> + if (*p & QOS_CONTROL_A_MSDU_PRESENT)
> + return 1;
> + else
> + return 0;
> +}
Hmm. Can we add the appropriate stuff to struct ieee80211_hdr in
include/linux/ieee80211.h and use that? Like
struct ieee80211_hdr *hdr = skb->data;
if (!(fc & ...))
return 0;
if (le16_to_cpu(hdr->qos_control) & QOS_CONTROL_A_MSDU_PRESENT)
return 1;
return 0;
Then again, this is a problem with the addr4 format, we'll probably have
to split up ieee80211_hdr into a 3-addr and a 4-addr format, or use a
union like this:
struct ieee80211_hdr {
__le16 frame_control;
__le16 duration_id;
__u8 addr1[6];
__u8 addr2[6];
__u8 addr3[6];
__le16 seq_ctrl;
union {
__u8 addr4[6]; /* keep for easier access */
struct {
__u8 addr4[6];
__le16 qos;
__le32 ht;
} _4addr;
struct {
__le16 qos;
__le32 ht;
} _3addr;
}
} __attribute__ ((packed));
Or something like that. Can you get frames w/o qos information but with
ht info? If so, we'll need even more cases here :/
Then again, how about we simply add a few inlines like
__le16 ieee80211_get_qos() that gives you the qos field etc, and then
use those. This seems dangerous anyway considering the possible presence
of the HT field.
> + hdrlen = ieee80211_get_hdrlen(fc);
I haven't seen a patch to ieee80211_get_hdrlen to add the HT field.
Isn't that missing?
> + payload = skb->data + hdrlen;
Ok so payload now points to the subframes.
> + if (unlikely(skb->len - hdrlen < 8)) {
> + if (net_ratelimit()) {
> + printk(KERN_DEBUG "%s: RX too short data frame "
> + "payload\n", dev->name);
Nit: this should increase the counter we have for too short frames
somewhere.
> + }
> + return TXRX_DROP;
> + }
> +
> + ethertype = (payload[6] << 8) | payload[7];
Is that really correct?
> + if (likely((memcmp(payload, rfc1042_header, 6) == 0 &&
> + ethertype != ETH_P_AARP && ethertype != ETH_P_IPX) ||
> + memcmp(payload, bridge_tunnel_header, 6) == 0)) {
> + /* remove RFC1042 or Bridge-Tunnel encapsulation and
> + * replace EtherType */
> + skb_pull(skb, hdrlen + 6);
> + } else {
> + skb_pull(skb, hdrlen);
> + }
> +
> + eth = (struct ethhdr*)skb->data;
So that now points to the first actual subframe ethhdr. I misread that
in my first comment. Why do you actually skb_pull on this skb? It would
probably be more efficient to just assign "eth" in the different cases
since that needs no memory write (eth will probably be kept in a
register). And you throw it away anyway...
> + while ((u8*)eth < skb->data + skb->len) {
And that compares the pointer, not the value at it as I thought. Sorry.
> + unsigned int eth_len = sizeof(struct ethhdr) +
> + ntohs(eth->h_proto);
rename to subframe_len maybe?
> + frame = dev_alloc_skb(3000);
> +
> + if (frame == NULL)
> + return TXRX_DROP;
> +
> + frame->mac.raw = frame->data;
> + memcpy(skb_put(frame, eth_len), (u8*)eth, eth_len);
Nope, not nice to do this. You really need to check if eth_len doesn't
exceed the original skb's length or people can crash this by sending you
short frames with bogus huge eth len embedded in it. And if you move the
check up a bit then you can reuse "skb" for the last subframe in the
case where people don't send us garbage:
int remaining = subframe_len - ((u8*)eth - skb->data);
if (remaining < 0)
/* idiots. should blacklist their mac address */
return TXRX_DROP;
if (remaining < 4 /* padding */) {
frame = skb;
skb = NULL;
skb_pull(blabla)
} else {
frame = dev_alloc_skb(...)
if (!frame)
return TXRX_DROP;
}
or something like that.
Don't you also need to set ((ethhdr*)frame->data)->h_proto to something
other than the length? Not exactly sure though, it seems eth_type_trans
can handle that and I don't have the 802.3 specs handy.
> + frame->dev = dev;
I think it'd be good to move that closer to the netif_rx.
> +
> + skb2 = NULL;
> +
> + sdata->stats.rx_packets++;
> + sdata->stats.rx_bytes += frame->len;
> +
> + if (local->bridge_packets &&
> + (sdata->type == IEEE80211_IF_TYPE_AP ||
> + sdata->type == IEEE80211_IF_TYPE_VLAN) &&
> + rx->u.rx.ra_match) {
> + if (is_multicast_ether_addr(frame->data)) {
Use a new ethhdr* variable for that, like subframe_hdr, and then use
subframe_hdr->h_dest. Makes it much clearer what's going on with these
subframes.
> + /* send multicast frames both to higher layers
> + * in local net stack and back to the wireless
> + * media */
> + skb2 = skb_copy(frame, GFP_ATOMIC);
> + if (!skb2)
> + printk(KERN_DEBUG "%s: failed to clone "
> + "multicast frame\n", dev->name);
> + } else {
> + struct sta_info *dsta;
> + dsta = sta_info_get(local, frame->data);
> + if (dsta && !dsta->dev) {
> + printk(KERN_DEBUG "Station with null "
> + "dev structure!\n");
> + } else if (dsta && dsta->dev == dev) {
> + /* Destination station is associated
> + * to this AP, so send the frame
> + * directly to it and do not pass
> + * the frame to local net stack.
> + */
> + skb2 = skb;
> + skb = NULL;
I'm pretty sure you mean frame here instead of skb in both cases and
this is a copy/paste error from the other data rx handler.
> + }
> + if (dsta)
> + sta_info_put(dsta);
> + }
> + }
> + if (frame) {
> + /* deliver to local stack */
> + frame->protocol = eth_type_trans(frame, dev);
> + memset(frame->cb, 0, sizeof(frame->cb));
That memset is useless on a newly allocated frame.
> + netif_rx(frame);
> + }
> +
> + if (skb2) {
> + /* send to wireless media */
> + skb2->protocol = __constant_htons(ETH_P_802_3);
> + skb2->mac.raw = skb2->nh.raw = skb2->data;
> + dev_queue_xmit(skb2);
> + }
> +
> + eth = (struct ethhdr*)((u8*)eth + eth_len
> + + calc_pad_len(eth_len));
> + }
> +
> + dev_kfree_skb(skb);
> + return TXRX_QUEUED;
> +}
> +
> static ieee80211_txrx_result
> ieee80211_rx_h_data(struct ieee80211_txrx_data *rx)
> {
> @@ -4373,6 +4510,7 @@ static ieee80211_rx_handler ieee80211_rx
> ieee80211_rx_h_remove_qos_control,
> ieee80211_rx_h_802_1x_pae,
> ieee80211_rx_h_drop_unencrypted,
> + ieee80211_rx_h_data_agg,
> ieee80211_rx_h_data,
> ieee80211_rx_h_mgmt,
> NULL
> diff -Nupr wireless-dev/net/mac80211/wme.h
> wireless-dev-new/net/mac80211/wme.h
> --- wireless-dev/net/mac80211/wme.h 2007-03-22 11:40:17.000000000 -0700
> +++ wireless-dev-new/net/mac80211/wme.h 2007-03-27 01:48:34.000000000
> -0700
> @@ -24,6 +24,8 @@
>
> #define QOS_CONTROL_TAG1D_MASK 0x07
>
> +#define QOS_CONTROL_A_MSDU_PRESENT 0x80
Hm. Can we put all these things into <linux/ieee80211.h> instead? I'm ok
with you putting it here for now, we'll just move them later. Or you can
add a patch to your series before this one that already moves the
defines and adds this one.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
