Re: [Bridge] [PATCH v3 1/2] net: bridge: use mac_len in bridge forwarding

[Toshiaki Makita <toshiaki.makita1@gmail.com>]

On 2019/09/03 22:36, Zahari Doychev wrote:
> On Tue, Sep 03, 2019 at 08:37:36PM +0900, Toshiaki Makita wrote:
>> Hi Zahari,
>>
>> Sorry for reviewing this late.
>>
>> On 2019/09/03 3:09, Zahari Doychev wrote:
>> ...
>>> @@ -466,13 +466,14 @@ static bool __allowed_ingress(const struct net_bridge *br,
>>>    		/* Tagged frame */
>>>    		if (skb->vlan_proto != br->vlan_proto) {
>>>    			/* Protocol-mismatch, empty out vlan_tci for new tag */
>>> -			skb_push(skb, ETH_HLEN);
>>> +			skb_push(skb, skb->mac_len);
>>>    			skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>>>    							skb_vlan_tag_get(skb));
>>
>> I think we should insert vlan at skb->data, i.e. mac_header + mac_len, while this
>> function inserts the tag at mac_header + ETH_HLEN which is not always the correct
>> offset.
> 
> Maybe I am misunderstanding the concern here but this should make sure that
> the VLAN tag from the skb is move back in the payload as the outer most tag.
> So it should follow the ethernet header. It looks like this e.g.,:
> 
> VLAN1 in skb:
> +------+------+-------+
> | DMAC | SMAC | ETYPE |
> +------+------+-------+
> 
> VLAN1 moved to payload:
> +------+------+-------+-------+
> | DMAC | SMAC | VLAN1 | ETYPE |
> +------+------+-------+-------+
> 
> VLAN2 in skb:
> +------+------+-------+-------+
> | DMAC | SMAC | VLAN1 | ETYPE |
> +------+------+-------+-------+
> 
> VLAN2 moved to payload:
> 
> +------+------+-------+-------+
> | DMAC | SMAC | VLAN2 | VLAN1 | ....
> +------+------+-------+-------+
> 
> Doing the skb push with mac_len makes sure that VLAN tag is inserted in the
> correct offset. For mac_len == ETH_HLEN this does not change the current
> behaviour.

Reordering VLAN headers here does not look correct to me. If skb->data points to ETH+VLAN,
then we should insert the vlan at the offset.
Vlan devices with reorder_hdr disabled produce packets whose mac_len includes ETH+VLAN header,
and they expects vlan insertion after the outer vlan header.

Also I'm not sure there is standard ethernet header in mac_len, as mac_len is not ETH_HLEN.
E.g. tun devices can produce vlan packets without ehternet header.

> 
>>
>>>    			if (unlikely(!skb))
>>>    				return false;
>>>    			skb_pull(skb, ETH_HLEN);
>>
>> Now skb->data is mac_header + ETH_HLEN which would be broken when mac_len is not
>> ETH_HLEN?
> 
> I thought it would be better to point in this case to the outer tag as otherwise
> if mac_len is used the skb->data will point to the next tag which I find somehow
> inconsistent or do you see some case where this can cause problems?

Vlan devices with reorder_hdr off will break because it relies on skb->data offset
as I described in the previous discussion.

Toshiaki Makita