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

[Zahari Doychev <zahari.doychev@linux.com>]

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