netdev - Re: [PATCH v9 net-next 05/12] bridge: Add the ability to configure pvid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 01 Feb 2013 21:22:38 -0500
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Michał Mirosław <mirqus@...il.com>
CC:	shemminger@...tta.com, bridge@...ts.linux-foundation.org,
	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v9 net-next 05/12] bridge: Add the ability to configure
 pvid

On 02/01/2013 08:15 PM, Michał Mirosław wrote:
> 2013/2/2 Michał Mirosław <mirqus@...il.com>:
>> 2013/2/1 Vlad Yasevich <vyasevic@...hat.com>:
>>> A user may designate a certain vlan as PVID.  This means that
>>> any ingress frame that does not contain a vlan tag is assigned to
>>> this vlan and any forwarding decisions are made with this vlan in mind.
>> [...]
>>>   struct net_port_vlans {
>>>          u16                             port_idx;
>>> +       u16                             pvid;
>>
>> I'm confused about the implementation. I would expect pvid field in
>> net_bridge_port and adding a tag if it isn't there on ingress path.
>> The rest would be just like without PVIDs. But here you pvid field to
>> net_port_vlans, and don't do anything with it in receive nor transmit
>> path. Does it work? What am I missing?
>
> Found the answer in next patch (you should merge #5 and #6).

It was split for incremental testing.  #5 added the ability to set and
delete it without impacting anything.  #6 added the actual work that 
pvid does.

> Still,
> the implementation looks overly complicated. If you force the packet
> to canonical form on ingress (keeping outer tag in skb->vlan_tci, and
> setting skb->vlan_tci = pvid if there is no tag) the code should get
> simpler.

What if there is no outer tag?  That's what the ingress code is doing.
If there is no outer tag, pvid is written to vlan_tci.  If there was
outer tag in vlan_tci, it's left alone.  This way at the end of ingress
vlan_tci is always set.
At egress, we grab that tag and compare it against pvid if any.  If it
matches, it's stripped.  If it doesn't, we output with the tag thus
adding the header.

The only thing I can simplify is grab the tci directly at egress, but
that's what the code will do anyway.

-vlad

>
> Best Regards,
> Michał Mirosław
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html