Linux Ethernet Bridging - Sep 2019 - [Bridge] [PATCH v3 1/2] net: bridge: use mac_len in bridge forwarding

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.
> 
> >   			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?

> 
> > +			skb_reset_network_header(skb);
> >   			skb_reset_mac_len(skb);
> >   			*vid = 0;
> >   			tagged = false;
> > 
> 
> Toshiaki Makita

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