netdev - RE: [PATCH] ixgb: Convert to new vlan model.

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, 14 Dec 2010 12:08:03 -0700
From:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	Jesse Gross <jesse@...ira.com>, David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
Subject: RE: [PATCH] ixgb: Convert to new vlan model.

Ben Hutchings wrote:
> On Tue, 2010-12-14 at 11:09 -0700, Tantilov, Emil S wrote:
>> Ben Hutchings wrote:
>>> On Mon, 2010-12-13 at 19:42 -0800, Jesse Gross wrote:
>>>> This switches the ixgb driver to use the new vlan interfaces.
>>>> In doing this, it completes the work begun in
>>>> ae54496f9e8d40c89e5668205c181dccfa9ecda1 allowing the use of
>>>> hardware vlan insertion without having a vlan group configured.
>>>> [...] diff --git a/drivers/net/ixgb/ixgb_ethtool.c
>>>> b/drivers/net/ixgb/ixgb_ethtool.c
>>>> index 43994c1..0e4c527 100644
>>>> --- a/drivers/net/ixgb/ixgb_ethtool.c
>>>> +++ b/drivers/net/ixgb/ixgb_ethtool.c
>>>> @@ -706,6 +706,45 @@ ixgb_get_strings(struct net_device *netdev,
>>>> u32  stringset, u8 *data)  	} } 
>>>> 
>>>> +static int ixgb_set_flags(struct net_device *netdev, u32 data) +{
>>>> +	struct ixgb_adapter *adapter = netdev_priv(netdev); +	bool
>>>> need_reset; +	int rc; +
>>>> +	/* The hardware requires that RX vlan stripping and TX vlan
>>>> insertion +	 * be configured together.  Therefore, if one setting
>>>> changes adjust the +	 * other one to match. +	 */
>>>> +	if (!!(data & ETH_FLAG_RXVLAN) != !!(data & ETH_FLAG_TXVLAN)) {
>>>> +		if ((data & ETH_FLAG_RXVLAN) !=
>>>> +		    (netdev->features & NETIF_F_HW_VLAN_RX))
>>>> +			data ^= ETH_FLAG_TXVLAN;
>>>> +		else if ((data & ETH_FLAG_TXVLAN) !=
>>>> +		    (netdev->features & NETIF_F_HW_VLAN_TX))
>>>> +			data ^= ETH_FLAG_RXVLAN;
>>>> +	}
>>> [...]
>>> 
>>> I think this should reject attempts to change just one flag with
>>> -EINVAL, rather than quietly 'fixing' the setting.
>>> 
>>> Ben.
>> 
>> I'm not sure this is a good idea. At least not without some sort of
>> explanation. Since there is no way for the user to know that he needs
>> to disable both.
> 
> Document the limitation in Documentation/networking/ixgb.txt.  You
> could also send a patch for the ethtool manual page stating that this
> restriction might exist.
> 
> Ben.

Just to make sure it's clear - there is no hard requirement for both 
settings to be set at the same time. So setting:
ethtool -K eth0 rxvlan off

Is a valid setting and will disable stripping on Rx, but because of 
the design, stripping on Tx will also be disabled.

In order for Tx tag insertion to work:
CTRL0.VME = 1 and VLE in TX desc = 1

In order for Rx tag stripping to work:
CTRL0.VME = 1

Based on this I think it's justified to adjust Tx when Rx is disabled as
it basically reflects the actual state of affairs.

But we should allow Tx to be disabled separately of Rx (CTRL0.VME=1, VLE=0/1).

Either way we should update the docs as it is not intuitive.

Thanks,
Emil