Re: Headroom for bridging and wireless

I tried setting br->dev->hard_header_len to the max of the ports (see
attached patch).  Then I hardcoded hard_header_len to ETH_HLEN+4 for the
wireless driver.  I verified the kernel messages -- the bridge sets hh_len
to 18 when I add the wireless device to the bridge and back to 14 when I
remove it.  Unfortunately, it doesn't seem to be working.  Both bridged
traffic and traffic from the bridge itself still has 0 headroom in the
wireless tx function.  :-(

Here's my wireless code:

        ... snip ...
        if (iswep) {
                if (skb_headroom(skb) < IEEE80211_WEP_HLEN) {
                        /* NB: WEP headroom is 4, so no alignment problems. */
                        struct sk_buff *nskb;
                        printk(KERN_INFO "ath_tx_start: forced to realloc headroom (have %d, need %d)\n", skb_headroom(skb), IEEE80211_WEP_HLEN);
                        nskb = skb_realloc_headroom(skb, IEEE80211_WEP_HLEN);
                        dev_kfree_skb(skb);
                        if (nskb == NULL) {
                                return ENOMEM;
                        }
                        skb = nskb;
                        wh = (struct ieee80211_frame*)skb->data;
                }
        ... snip ...

On Sat, Dec 06, 2003 at 11:37:10AM +0100, Bart De Schuymer wrote:
> On Saturday 06 December 2003 01:08, Tom Marshall wrote:
> > I don't see anything like this in the bridge code.  Is that because
> > netfilter has to work across different media types but the bridge must use
> > ethernet devices?  If that is the case, is it valid to have an ethernet
> > device with hard_header_len != ETH_HLEN (eg. is this just a special case
> > that the bridge should not need to worry about)?
> 
> net/bridge/br_device.c::br_dev_setup() calls 
> drivers/net/net_init.c::ether_setup() when a new bridge device is created.
> 
> > Suppose we did set the bridge device's hard_header_len to the max of all
> > its interfaces as you suggested.  How would the headroom get set?  Does the
> > packet always go through the above netfilter code?
> 
> No.
> For routed IP traffic, the check is always done in 
> net/ipv4/ip_output.c::ip_finish_output2().
> 
> > Suppose the bridge code does check the headroom and the madwifi driver does
> > set hard_header_len == ETH_HLEN+4.  The original sk_buff is allocated by
> > the incoming ethernet driver, correct?  So it won't have enough headroom
> > and we still wind up doing a realloc/copy (just in a different place)
> > right?
> 
> Yes, I was thinking about packets originating from the bridge box itself.
> The vlan code does this when creating a new vlan device 
> (net/8021q/vlan.c::register_vlan_device()):
> 	new_dev->hard_header_len = real_dev->hard_header_len;
> 	if (!(real_dev->features & NETIF_F_HW_VLAN_TX)) {
> 		/* Regular ethernet + 4 bytes (18 total). */
> 		new_dev->hard_header_len += VLAN_HLEN;
> 	}
> 
> To deal with bridge packets, the madwifi code will need a test like this in 
> the tx code:
> if (skb_headroom(skb) < VLAN_HLEN) {
> 	struct sk_buff *sk_tmp = skb;
> 	skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
> }
> 
> This is also done in the vlan code, see 
> net/8021q/vlan_dev.c::vlan_dev_hard_start_xmit().
> 
> > Wandering further away from bridge questions...
> >
> > When a packet hits the device tx routine, can the device change the skb?
> > More specifically, is it valid to call skb_push() for the WEP header and
> > change the skb->data pointer?  I'm thinking the answer is yes, this is a
> > valid thing to do, because the netfilter code does it.  But I would like to
> > know for sure -- maybe netfilter makes a copy and the bridge code doesn't.
> 
> Yes.
> 
> > And finally, the wireless device can change its WEP setting at any time.
> > Is it valid to change hard_header_len anytime, or do we need to set it to
> > ETH_HLEN+4 when opening the device and leave it there?
> 
> You can change it, but you will probably need the call_rcu mechanism to be 
> sure this size isn't changed at the wrong time. Note that changing this value 
> would mean the hard_header_len of the bridge device will possibly need to be 
> changed too.
> You'd better consult with Stephen Hemminger if he would accept the changes to 
> be made to the bridge code.
> 
> cheers,
> Bart

-- 
Security-wise, NT is a server with a "Kick me" sign taped to it. 
        -- Peter Gutmann

--- /var/src/linux-2.6.0-test11/net/bridge/br_if.c	2003-11-26 12:45:53.000000000 -0800
+++ /var/src/linux-2.6.0-test11-dev/net/bridge/br_if.c	2003-12-06 15:08:56.000000000 -0800
@@ -224,6 +224,11 @@
 		return -EXFULL;
 	}
 
+	if (dev->hard_header_len > br->dev->hard_header_len) {
+		printk(KERN_INFO "br_add_if: setting hhl to %hu for %s\n", dev->hard_header_len, dev->name);
+		br->dev->hard_header_len = dev->hard_header_len; /* XXX locking */
+	}
+
 	dev_set_promiscuity(dev, 1);
 
 	br_stp_recalculate_bridge_id(br);
@@ -238,11 +243,24 @@
 int br_del_if(struct net_bridge *br, struct net_device *dev)
 {
 	struct net_bridge_port *p;
+	unsigned short hh_len;
 
-	if ((p = dev->br_port) == NULL || p->br != br)
+	if (dev->br_port == NULL || dev->br_port->br != br)
 		return -EINVAL;
 
-	del_nbp(p);
+	/* recalc hard_header_len: max of all remaining ports */
+	hh_len = ETH_HLEN;
+	list_for_each_entry(p, &br->port_list, list) {
+		if (p->dev != dev) {
+			hh_len = max(hh_len, p->dev->hard_header_len);
+		}
+	}
+	if (br->dev->hard_header_len != hh_len) {
+		printk(KERN_INFO "br_del_if: resetting hhl to %hu for %s\n", hh_len, dev->name);
+		br->dev->hard_header_len = hh_len; /* XXX locking */
+	}
+
+	del_nbp(dev->br_port);
 	br_stp_recalculate_bridge_id(br);
 	return 0;
 }

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iEYEARECAAYFAj/SZLYACgkQFMm9uvwPXW67GACeJA7ZzmTF7BW/QaKEhJsg1SD5
mLgAn1hX/95MFO8W8dRERXJfkFtT+QCC
=cilQ
-----END PGP SIGNATURE-----