On Sun, 2019-06-16 at 11:51 +0300, Nikolay Aleksandrov wrote: > > Thinking along those lines, I sort of ended up with the following scheme > > (just for the skb head, not the frags/fraglist): > > > > +------------------+----------------+---------------+ > > headroom | eth | vlan | ... | IP | TCP | payload | tailroom > > +------------------+----------------+---------------+ > > ^ skb->head_ptr > > ^ skb->l2_ptr > > ^ skb->l3_ptr == skb->l2_ptr + skb->l2_len > > ... > > ^ skb->payload_ptr > > ^ skb->tail [...] > > (Now, if you wanted to implement this, you probably wouldn't have l2_ptr > > but l2_offset etc. but that's an implementation detail.) > > > > I do like the scheme outlined above, it makes it easier to reason about > all of this, but obviously it'd require quite some changes. Yeah. I'm not really ready to suggest something as radical. But as you found out below, I even got confused *again* while *carefully* looking at this, and messed up mac_len vs. mac_header_len. In fact, even looking at it now, I'm not entirely sure I see the difference. Why do we need both? They have different implementation semantics, but shouldn't they sort of be the same? > > > It breaks connectivity between bridge and > > > members when vlans are used. The host generated packets going out of the bridge > > > have mac_len = 0. > > > > Which probably indicates that we're not even consistent with the egress > > scheme I pointed out above, probably because we *also* have > > hard_header_len? > > > > IIRC, mac_len is only set on Rx, while on Tx it usually isn't. More below. Yes, looks like. > > I'm not even sure I understand the bug that Nikolay described, because > > br_dev_xmit() does: > > > > skb_reset_mac_header(skb); > > eth = eth_hdr(skb); > > skb_pull(skb, ETH_HLEN); > > > > so after this we *do* end up with an SKB that has mac_len == ETH_HLEN, > > if it was transmitted out the bridge netdev itself, and thus how would > > the bug happen? > > > > I said *mac_len*. :) Yes, I confused myself here. > The above sets mac_header, at that point you'll have > the following values: mac_len = 0, mac_header_len = 14 (skb_mac_header_len > uses network_header - mac_header which is set there), but that is easy > to overcome and if you do go down the path of consistently using and updating > mac_len it should work. Yeah, so basically all we really need is to actually call skb_reset_mac_len() in addition to skb_reset_mac_header(). Which, is, "slightly" confusing (to say the least) - why are mac_len and mac_header two completely separate concepts? It almost seems like they should be two sides of the same coin (len/ptr) but we also have mac_header_len... Oh well. So maybe we should go back to square 1 and resend the patches Zahari had originally, but with the added skb_reset_mac_len()? johannes
