Hi Alexei, Sorry for messing up your address btw, not sure where I dug that one up...> > 1) Make the bridge code use skb->mac_len instead of ETH_HLEN. This > > works for this particular case, but breaks some other cases; > > evidently some places exist where skb->mac_len isn't even set to > > ETH_HLEN when a packet gets to the bridge. I don't know right now > > what that was, I think probably somebody who's CC'ed reported that. > > > > 2) Let tc_act_vlan() just pull ETH_HLEN instead of skb->mac_len, but > > this is rather asymmetric and strange, and while it works for this > > case it may cause confusion elsewhere. > > > > 2b) Toshiaki said it might be better to make that code *remember* > > mac_len and then use it to push and pull (so not caring about the > > change made by skb_vlan_push()), but that ultimately leads to > > confusion and if you have TC push/pop combinations things just get > > completely out of sync and die > > > > 3) Make skb_vlan_push()/_pop() just not change mac_len at all. So far > > this also addresses the issue, but it's likely that this will break > > OVS, and I don't know how it'd affect BPF. Quite possibly like TC > > does and is broken, but perhaps not. > > > > > > But now we're stuck. Depending on how you look at it, all of these seem > > sort of reasonable, or not. > > > > Ultimately, the issue seems to be that we couldn't really decide whether > > VLAN tags (and probably MPLS tags, for that matter) are covered by > > mac_len or not. At least not consistently on ingress and egress. > > eth_type_trans() doesn't take them into account, so of course on simple > > ingress mac_len will only cover the ETH_HLEN. > > > > If you have an accelerated tag and then push it into the SKB, it will > > *not* be taken into account in mac_len. OTOH, if you have a new tag and > > use skb_vlan_push() then it *will* be taken into account. > > > > > > I'm trending towards solution (3), because if we consider other > > combinations of VLAN push/pop in TC, I think we can end up in a very > > messy situation today. For example, POP/PUSH seems like it should be a > > no-op, but it isn't due to the mac_len, *unless* it can use the HW accel > > only (i.e. only a single tag). > > > > I think then to propose such a patch though we'd have to figure out > > where the BPF case is, and to keep OVS working probably either add an > > argument ("bool adjust_mac_len") to the function signatures, or just do > > the adjustments in OVS code after calling them? > > > > > > Any other thoughts? > > imo skb_vlan_push() should still change mac_len. > tc, ovs, bpf use it and expect vlan to be part of L2.I'm not sure tc really cares, but it *is* a reasonable argument to make. Like I said, whichever way I look at the problem, a different solution looks more reasonable ;-)> There is nothing between L2 and L3 :) > Hence we cannot say that vlan is not part of L2. > Hence push/pop vlan must change mac_len, since skb->mac_len > is kernel's definition of the length of L2 header.I think we're getting to something here now. I actually thought about this some more last night, and basically asked myself how I would design it without all the legacy baggage. I'm certainly not suggesting we should change anything here, but to me it was a bit of a clarification to do this and then see where we differ in our handling today. 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, I deliberately didn't put any "skb->data" here, because what we do today is sort of confusing. By getting rid of the "multi-use" skb->data in this scheme I think it becomes clearer to think about. On *egress*, all we really care about is this: +------------------+----------------+---------------+ headroom | eth | vlan | ... | IP / TCP | payload | tailroom +------------------+----------------+---------------+ ^ skb->data skb->data + skb->len ^ On *ingress*, however, we hide some of the data (temporarily): |--------- headroom ---------| +------------------+----------------+---------------+ | eth | vlan | ... | IP / TCP | payload | tailroom +------------------+----------------+---------------+ ^ skb->data - skb->mac_len ^ skb->data skb->data + skb->len ^ which is somewhat confusing to me, and sort of causes all these problems. (It also makes it harder to reason about what data is actually valid in the skb, although if mac_len is non-zero then it must be, but it means you actually have less headroom and all). If instead we just made it like the hypothetical scheme I outlined above, then on traversing a layer we'd set the next layer pointer appropriately, and then each layer would just use the right pointer: * bridge/ethernet driver/... would use l2_ptr * IP would use the l3_ptr * TCP would use the l4_ptr (didn't put that into the picture) * ... Now we wouldn't have a problem with the VLAN tags, because we'd just appropriate set/keep all the pointers - bridge doesn't even care where l3_ptr is pointing, but for IP it would of course point to beyond the VLAN tags. (Now, if you wanted to implement this, you probably wouldn't have l2_ptr but l2_offset etc. but that's an implementation detail.) Now, why am I writing all this? Because I think it points out that you're absolutely right - we should treat mac_len as part of the frame if we're in anything that cares about L2 like bridge.> Now as far as bridge... I think it's unfortunate that linux > adopted 'vlan' as a netdevice model and that's where I think > the problem is.I'm not sure. I don't exactly know where the problem is if we fix bridge according to the patch (1) above, which, btw, was discussed before: https://lore.kernel.org/netdev/20190113135939.8970-1-zahari.doychev at linux.com/ Back then, Nikolay (whom I forgot to CC, fixed now) said:> 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? Maybe somewhere early in the egress path we should set skb->mac_len to dev->hard_header_len, and then use skb->mac_len consistently, and consider that part of the skb (and not arbitrarily consider ETH_HLEN to be part of the skb in bridge). (This almost tempts me to actually try to implement the hypothetical SKB scheme I described above, just so it's easier to understand what part does what ... and to find where the issues like this occur)> Typical bridge in the networking industry is a device that > does forwarding based on L2. Which includes vlans. > And imo that's the most appropriate way of configuring and thinking > about bridge functionality. > Whereas in the kernel there is a 'vlan' netdevice that 'eats' > vlan tag and pretends that the rest is the same. > So linux bridge kinda doesn't need to be vlan aware. > CONFIG_BRIDGE_VLAN_FILTERING was the right step, but I haven't > seen it being used and I'm not sure about state of things there.I think that ends up being a question of semantics. You can consider an "industry bridge" that you describe to be a combination of VLAN and bridge netdevs, and so it's just a question of what exactly you consider a "bridge" - does it have to be a single netdev or not.> So your option 1 above is imo the best. The bridge needs to deal > with skb->mac_len and full L2 header.Yeah, I guess. We're back to square 1 ;-) 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? Thanks, johannes
On 15/06/2019 22:19, Johannes Berg wrote:> Hi Alexei, > > Sorry for messing up your address btw, not sure where I dug that one > up... > >>> 1) Make the bridge code use skb->mac_len instead of ETH_HLEN. This >>> works for this particular case, but breaks some other cases; >>> evidently some places exist where skb->mac_len isn't even set to >>> ETH_HLEN when a packet gets to the bridge. I don't know right now >>> what that was, I think probably somebody who's CC'ed reported that. >>> >>> 2) Let tc_act_vlan() just pull ETH_HLEN instead of skb->mac_len, but >>> this is rather asymmetric and strange, and while it works for this >>> case it may cause confusion elsewhere. >>> >>> 2b) Toshiaki said it might be better to make that code *remember* >>> mac_len and then use it to push and pull (so not caring about the >>> change made by skb_vlan_push()), but that ultimately leads to >>> confusion and if you have TC push/pop combinations things just get >>> completely out of sync and die >>> >>> 3) Make skb_vlan_push()/_pop() just not change mac_len at all. So far >>> this also addresses the issue, but it's likely that this will break >>> OVS, and I don't know how it'd affect BPF. Quite possibly like TC >>> does and is broken, but perhaps not. >>> >>> >>> But now we're stuck. Depending on how you look at it, all of these seem >>> sort of reasonable, or not. >>> >>> Ultimately, the issue seems to be that we couldn't really decide whether >>> VLAN tags (and probably MPLS tags, for that matter) are covered by >>> mac_len or not. At least not consistently on ingress and egress. >>> eth_type_trans() doesn't take them into account, so of course on simple >>> ingress mac_len will only cover the ETH_HLEN. >>> >>> If you have an accelerated tag and then push it into the SKB, it will >>> *not* be taken into account in mac_len. OTOH, if you have a new tag and >>> use skb_vlan_push() then it *will* be taken into account. >>> >>> >>> I'm trending towards solution (3), because if we consider other >>> combinations of VLAN push/pop in TC, I think we can end up in a very >>> messy situation today. For example, POP/PUSH seems like it should be a >>> no-op, but it isn't due to the mac_len, *unless* it can use the HW accel >>> only (i.e. only a single tag). >>> >>> I think then to propose such a patch though we'd have to figure out >>> where the BPF case is, and to keep OVS working probably either add an >>> argument ("bool adjust_mac_len") to the function signatures, or just do >>> the adjustments in OVS code after calling them? >>> >>> >>> Any other thoughts? >> >> imo skb_vlan_push() should still change mac_len. >> tc, ovs, bpf use it and expect vlan to be part of L2. > > I'm not sure tc really cares, but it *is* a reasonable argument to make. > > Like I said, whichever way I look at the problem, a different solution > looks more reasonable ;-) > >> There is nothing between L2 and L3 :) >> Hence we cannot say that vlan is not part of L2. >> Hence push/pop vlan must change mac_len, since skb->mac_len >> is kernel's definition of the length of L2 header. > > I think we're getting to something here now. I actually thought about > this some more last night, and basically asked myself how I would design > it without all the legacy baggage. > > I'm certainly not suggesting we should change anything here, but to me > it was a bit of a clarification to do this and then see where we differ > in our handling today. > > 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, I deliberately didn't put any "skb->data" here, because what we do > today is sort of confusing. > > By getting rid of the "multi-use" skb->data in this scheme I think it > becomes clearer to think about. > > > > On *egress*, all we really care about is this: > > +------------------+----------------+---------------+ > headroom | eth | vlan | ... | IP / TCP | payload | tailroom > +------------------+----------------+---------------+ > > ^ skb->data > skb->data + skb->len > ^ > > On *ingress*, however, we hide some of the data (temporarily): > > |--------- headroom ---------| > +------------------+----------------+---------------+ > | eth | vlan | ... | IP / TCP | payload | tailroom > +------------------+----------------+---------------+ > ^ skb->data - skb->mac_len > ^ skb->data > skb->data + skb->len ^ > > which is somewhat confusing to me, and sort of causes all these > problems. > > (It also makes it harder to reason about what data is actually valid in > the skb, although if mac_len is non-zero then it must be, but it means > you actually have less headroom and all). > > If instead we just made it like the hypothetical scheme I outlined > above, then on traversing a layer we'd set the next layer pointer > appropriately, and then each layer would just use the right pointer: > > * bridge/ethernet driver/... would use l2_ptr > * IP would use the l3_ptr > * TCP would use the l4_ptr (didn't put that into the picture) > * ... > > Now we wouldn't have a problem with the VLAN tags, because we'd just > appropriate set/keep all the pointers - bridge doesn't even care where > l3_ptr is pointing, but for IP it would of course point to beyond the > VLAN tags. > > (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.> Now, why am I writing all this? Because I think it points out that > you're absolutely right - we should treat mac_len as part of the frame > if we're in anything that cares about L2 like bridge. > >> Now as far as bridge... I think it's unfortunate that linux >> adopted 'vlan' as a netdevice model and that's where I think >> the problem is. > > I'm not sure. I don't exactly know where the problem is if we fix bridge > according to the patch (1) above, which, btw, was discussed before: > https://lore.kernel.org/netdev/20190113135939.8970-1-zahari.doychev at linux.com/ > > Back then, Nikolay (whom I forgot to CC, fixed now) said: > >> 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.> Maybe somewhere early in the egress path we should set skb->mac_len to > dev->hard_header_len, and then use skb->mac_len consistently, and > consider that part of the skb (and not arbitrarily consider ETH_HLEN to > be part of the skb in bridge). > > (This almost tempts me to actually try to implement the hypothetical SKB > scheme I described above, just so it's easier to understand what part > does what ... and to find where the issues like this occur) >Interesting idea.>> Typical bridge in the networking industry is a device that >> does forwarding based on L2. Which includes vlans. >> And imo that's the most appropriate way of configuring and thinking >> about bridge functionality. >> Whereas in the kernel there is a 'vlan' netdevice that 'eats' >> vlan tag and pretends that the rest is the same. >> So linux bridge kinda doesn't need to be vlan aware. >> CONFIG_BRIDGE_VLAN_FILTERING was the right step, but I haven't >> seen it being used and I'm not sure about state of things there. > > I think that ends up being a question of semantics. You can consider an > "industry bridge" that you describe to be a combination of VLAN and > bridge netdevs, and so it's just a question of what exactly you consider > a "bridge" - does it have to be a single netdev or not. > >> So your option 1 above is imo the best. The bridge needs to deal >> with skb->mac_len and full L2 header. > > Yeah, I guess. We're back to square 1 ;-) > > 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*. :) 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. Cheers, Nik> Thanks, > johannes >
On 06/15/2019 09:19 PM, Johannes Berg wrote:> Hi Alexei, > > Sorry for messing up your address btw, not sure where I dug that one > up... > >>> 1) Make the bridge code use skb->mac_len instead of ETH_HLEN. This >>> works for this particular case, but breaks some other cases; >>> evidently some places exist where skb->mac_len isn't even set to >>> ETH_HLEN when a packet gets to the bridge. I don't know right now >>> what that was, I think probably somebody who's CC'ed reported that. >>> >>> 2) Let tc_act_vlan() just pull ETH_HLEN instead of skb->mac_len, but >>> this is rather asymmetric and strange, and while it works for this >>> case it may cause confusion elsewhere. >>> >>> 2b) Toshiaki said it might be better to make that code *remember* >>> mac_len and then use it to push and pull (so not caring about the >>> change made by skb_vlan_push()), but that ultimately leads to >>> confusion and if you have TC push/pop combinations things just get >>> completely out of sync and die >>> >>> 3) Make skb_vlan_push()/_pop() just not change mac_len at all. So far >>> this also addresses the issue, but it's likely that this will break >>> OVS, and I don't know how it'd affect BPF. Quite possibly like TC >>> does and is broken, but perhaps not. >>> >>> >>> But now we're stuck. Depending on how you look at it, all of these seem >>> sort of reasonable, or not. >>> >>> Ultimately, the issue seems to be that we couldn't really decide whether >>> VLAN tags (and probably MPLS tags, for that matter) are covered by >>> mac_len or not. At least not consistently on ingress and egress. >>> eth_type_trans() doesn't take them into account, so of course on simple >>> ingress mac_len will only cover the ETH_HLEN. >>> >>> If you have an accelerated tag and then push it into the SKB, it will >>> *not* be taken into account in mac_len. OTOH, if you have a new tag and >>> use skb_vlan_push() then it *will* be taken into account. >>> >>> >>> I'm trending towards solution (3), because if we consider other >>> combinations of VLAN push/pop in TC, I think we can end up in a very >>> messy situation today. For example, POP/PUSH seems like it should be a >>> no-op, but it isn't due to the mac_len, *unless* it can use the HW accel >>> only (i.e. only a single tag). >>> >>> I think then to propose such a patch though we'd have to figure out >>> where the BPF case is, and to keep OVS working probably either add an >>> argument ("bool adjust_mac_len") to the function signatures, or just do >>> the adjustments in OVS code after calling them? >>> >>> >>> Any other thoughts? >> >> imo skb_vlan_push() should still change mac_len. >> tc, ovs, bpf use it and expect vlan to be part of L2. > > I'm not sure tc really cares, but it *is* a reasonable argument to make. > > Like I said, whichever way I look at the problem, a different solution > looks more reasonable ;-)Agree with Alexei that the approach which would be least confusing and/or potentially causing regressions might be to go for 1). tc *does* care at least for the *BPF* case. In sch_clsact we have the ingress and egress hook where we can attach to and programs don't need to care where they are being attached since for both cases they see skb->data starting at eth header! In order to do this, we do a __skb_push()/__skb_pull() dance based on skb->mac_len depending from where we come. This also means that if a program pushed/popped a vlan tag, this still must be correct wrt expectations for the receive side. It is essential that there is consistent behavior on skb->mac_len given skbs can also be redirected from TX->RX or RX->TX(->RX), so that we don't pull to a wrong offset next time. Thanks, Daniel
On 2019/06/16 4:19, Johannes Berg wrote:> Hi Alexei, > > Sorry for messing up your address btw, not sure where I dug that one > up... > >>> 1) Make the bridge code use skb->mac_len instead of ETH_HLEN. This >>> works for this particular case, but breaks some other cases; >>> evidently some places exist where skb->mac_len isn't even set to >>> ETH_HLEN when a packet gets to the bridge. I don't know right now >>> what that was, I think probably somebody who's CC'ed reported that. >>> >>> 2) Let tc_act_vlan() just pull ETH_HLEN instead of skb->mac_len, but >>> this is rather asymmetric and strange, and while it works for this >>> case it may cause confusion elsewhere. >>> >>> 2b) Toshiaki said it might be better to make that code *remember* >>> mac_len and then use it to push and pull (so not caring about the >>> change made by skb_vlan_push()), but that ultimately leads to >>> confusion and if you have TC push/pop combinations things just get >>> completely out of sync and die >>> >>> 3) Make skb_vlan_push()/_pop() just not change mac_len at all. So far >>> this also addresses the issue, but it's likely that this will break >>> OVS, and I don't know how it'd affect BPF. Quite possibly like TC >>> does and is broken, but perhaps not. >>> >>> >>> But now we're stuck. Depending on how you look at it, all of these seem >>> sort of reasonable, or not. >>> >>> Ultimately, the issue seems to be that we couldn't really decide whether >>> VLAN tags (and probably MPLS tags, for that matter) are covered by >>> mac_len or not. At least not consistently on ingress and egress. >>> eth_type_trans() doesn't take them into account, so of course on simple >>> ingress mac_len will only cover the ETH_HLEN. >>> >>> If you have an accelerated tag and then push it into the SKB, it will >>> *not* be taken into account in mac_len. OTOH, if you have a new tag and >>> use skb_vlan_push() then it *will* be taken into account. >>> >>> >>> I'm trending towards solution (3), because if we consider other >>> combinations of VLAN push/pop in TC, I think we can end up in a very >>> messy situation today. For example, POP/PUSH seems like it should be a >>> no-op, but it isn't due to the mac_len, *unless* it can use the HW accel >>> only (i.e. only a single tag). >>> >>> I think then to propose such a patch though we'd have to figure out >>> where the BPF case is, and to keep OVS working probably either add an >>> argument ("bool adjust_mac_len") to the function signatures, or just do >>> the adjustments in OVS code after calling them? >>> >>> >>> Any other thoughts? >> >> imo skb_vlan_push() should still change mac_len. >> tc, ovs, bpf use it and expect vlan to be part of L2. > > I'm not sure tc really cares, but it *is* a reasonable argument to make. > > Like I said, whichever way I look at the problem, a different solution > looks more reasonable ;-) > >> There is nothing between L2 and L3 :) >> Hence we cannot say that vlan is not part of L2. >> Hence push/pop vlan must change mac_len, since skb->mac_len >> is kernel's definition of the length of L2 header. > > I think we're getting to something here now. I actually thought about > this some more last night, and basically asked myself how I would design > it without all the legacy baggage. > > I'm certainly not suggesting we should change anything here, but to me > it was a bit of a clarification to do this and then see where we differ > in our handling today. > > 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, I deliberately didn't put any "skb->data" here, because what we do > today is sort of confusing. > > By getting rid of the "multi-use" skb->data in this scheme I think it > becomes clearer to think about. > > > > On *egress*, all we really care about is this: > > +------------------+----------------+---------------+ > headroom | eth | vlan | ... | IP / TCP | payload | tailroom > +------------------+----------------+---------------+ > > ^ skb->data > skb->data + skb->len > ^ > > On *ingress*, however, we hide some of the data (temporarily): > > |--------- headroom ---------| > +------------------+----------------+---------------+ > | eth | vlan | ... | IP / TCP | payload | tailroom > +------------------+----------------+---------------+ > ^ skb->data - skb->mac_len > ^ skb->data > skb->data + skb->len ^ > > which is somewhat confusing to me, and sort of causes all these > problems. > > (It also makes it harder to reason about what data is actually valid in > the skb, although if mac_len is non-zero then it must be, but it means > you actually have less headroom and all). > > If instead we just made it like the hypothetical scheme I outlined > above, then on traversing a layer we'd set the next layer pointer > appropriately, and then each layer would just use the right pointer: > > * bridge/ethernet driver/... would use l2_ptr > * IP would use the l3_ptr > * TCP would use the l4_ptr (didn't put that into the picture) > * ... > > Now we wouldn't have a problem with the VLAN tags, because we'd just > appropriate set/keep all the pointers - bridge doesn't even care where > l3_ptr is pointing, but for IP it would of course point to beyond the > VLAN tags. > > (Now, if you wanted to implement this, you probably wouldn't have l2_ptr > but l2_offset etc. but that's an implementation detail.) > > Now, why am I writing all this? Because I think it points out that > you're absolutely right - we should treat mac_len as part of the frame > if we're in anything that cares about L2 like bridge. > >> Now as far as bridge... I think it's unfortunate that linux >> adopted 'vlan' as a netdevice model and that's where I think >> the problem is. > > I'm not sure. I don't exactly know where the problem is if we fix bridge > according to the patch (1) above, which, btw, was discussed before: > https://lore.kernel.org/netdev/20190113135939.8970-1-zahari.doychev at linux.com/ > > Back then, Nikolay (whom I forgot to CC, fixed now) said: > >> 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? > > Maybe somewhere early in the egress path we should set skb->mac_len to > dev->hard_header_len, and then use skb->mac_len consistently, and > consider that part of the skb (and not arbitrarily consider ETH_HLEN to > be part of the skb in bridge). > > (This almost tempts me to actually try to implement the hypothetical SKB > scheme I described above, just so it's easier to understand what part > does what ... and to find where the issues like this occur) > >> Typical bridge in the networking industry is a device that >> does forwarding based on L2. Which includes vlans. >> And imo that's the most appropriate way of configuring and thinking >> about bridge functionality. >> Whereas in the kernel there is a 'vlan' netdevice that 'eats' >> vlan tag and pretends that the rest is the same. >> So linux bridge kinda doesn't need to be vlan aware. >> CONFIG_BRIDGE_VLAN_FILTERING was the right step, but I haven't >> seen it being used and I'm not sure about state of things there. > > I think that ends up being a question of semantics. You can consider an > "industry bridge" that you describe to be a combination of VLAN and > bridge netdevs, and so it's just a question of what exactly you consider > a "bridge" - does it have to be a single netdev or not. > >> So your option 1 above is imo the best. The bridge needs to deal >> with skb->mac_len and full L2 header. > > Yeah, I guess. We're back to square 1 ;-)I'll try to explain the problem I see, which cannot be fixed by option 1... The bug is in tcf_vlan_act(), and mainly in skb->data, not in mac_len. Consider about vlan packets from NIC, but non-hw-accelerated, where vlan devices are configured to receive them. When __netif_receive_skb_core() is called, skb is like this. +-----+------+-------- | eth | vlan | TCP/IP +-----+------+-------- ^ data skb->data is at the beginning of the vlan header. This is reasonable because we did not process the vlan tag at this point. Then after vlan_do_receive() (receive the skb on a vlan device), the skb is like this. +-----+-------- | eth | TCP/IP +-----+-------- ^ data Or if reorder_hdr is off (which does not remove vlan tags when receiving on vlan devices), +-----+------+-------- | eth | vlan | TCP/IP +-----+------+-------- ^ data Relying on this mechanism, we are currently able to handle multiple vlan tags. For example if we have 2 tags, - On __netif_receive_skb_core() invocation +-----+------+------+-------- | eth | vlan | vlan | TCP/IP +-----+------+------+-------- ^ data - After first vlan_do_receive() +-----+------+-------- | eth | vlan | TCP/IP +-----+------+-------- ^ data Or if reorder_hdr is off, +-----+------+------+-------- | eth | vlan | vlan | TCP/IP +-----+------+------+-------- ^ data When we process one tag, the data goes forward by one tag. Now looking at TC vlan case... After it inserts two tags, the skb looks like: (The first tag is in vlan_tci) +-----+------+-------- | eth | vlan | TCP/IP +-----+------+-------- ^ data The data pointer went forward before we process it. This is apparently wrong. I think we don't want to (or cannot?) handle cases like this after tcf_vlan_act(). This is why I said we should remember mac_len there. So, my opinion is: On ingress, data pointer can be at the end of vlan header and mac_len probably should include vlan tag length, but only after the vlan tag is processed. Bridge may need to handle mac_len that is not equal to ETH_HLEN but to me it's a different problem. Toshiaki Makita