netdev - Re: [PATCH 1/2] vlan: only create special VLAN 0 once

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:	Tue, 7 Jun 2011 18:18:08 +0200
From:	Jiri Bohac <jbohac@...e.cz>
To:	David Miller <davem@...emloft.net>
Cc:	jbohac@...e.cz, kaber@...sh.net, netdev@...r.kernel.org,
	pedro.netdev@...devamos.com
Subject: Re: [PATCH 1/2] vlan: only create special VLAN 0 once

Hi David,

On Sun, Jun 05, 2011 at 02:28:23PM -0700, David Miller wrote:
> From: Jiri Bohac <jbohac@...e.cz>
> Date: Fri, 3 Jun 2011 22:07:38 +0200
> 
> > Commit ad1afb00 registers a VLAN with vid == 0 for every device to handle
> > 802.1p frames.  This is currently done on every NETDEV_UP event and the special
> > vlan is never unregistered.  This may have strange effects on drivers
> > implementning ndo_vlan_rx_add_vid(). E.g. bonding will allocate a linked-list
> > element each time, causing a memory leak.
> > 
> > Only register the special VLAN once on NETDEV_REGISTER.
> > 
> > Signed-off-by: Jiri Bohac <jbohac@...e.cz>
> 
> I recognize the problem, but this solution isn't all that good.
> 
> I am pretty sure that the hardware device driver methods that
> implement ndo_vlan_rx_add_vid() assume that the device is up.
> Because most drivers completely reset the chip when the
> interface is brought up and this will likely clear out the
> VLAN ID tables in the chip.

Really? In that case, we have a much bigger problem: the vlan
code allows registering a new vlan on an interface that is down.
And it only registers the VID with ndo_vlan_rx_add_vid() in
register_vlan_dev() during the registration of the new vlan
interface -- it never re-registers the VIDs on a NETDEV_UP.

That would mean doing:

	ip link set down eth0
	ip link add link eth0 name eth0.1 type vlan id 1
	ip link set up eth0

... should result in a non-working setup, right? I would expect
an -EINVAL somewhere along the way.
However, at least for e1000, I just tested the above setup works.

Could you be wrong about this? Or is this supposed to fail with
other chips?  In that case, the vlan code or the drivers need
fixing. Something should either disallow adding vlans when down
and disallow putting the interface down then vlans are
configured, or it should re-register the VIDs on every NETDEV_UP.

> Second, now even devices which don't ever get brought up will
> have the VLAN ID 0 thing allocated.

Why is this a problem?

> Probably the thing to do is to remove the VLAN ID 0 entry on
> NETDEV_DOWN.

OK, if you prefer fixing it this way, why not...

-- 
Jiri Bohac <jbohac@...e.cz>
SUSE Labs, SUSE CZ

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