netdev - RE: [net-next 08/12] 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, 25 Jan 2011 11:20:11 -0700
From:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
To:	Jesse Gross <jesse@...ira.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"bphilips@...ell.com" <bphilips@...ell.com>,
	"Pieper, Jeffrey E" <jeffrey.e.pieper@...el.com>,
	Ben Hutchings <bhutchings@...arflare.com>
Subject: RE: [net-next 08/12] ixgb: convert to new VLAN model

>-----Original Message-----
>From: Jesse Gross [mailto:jesse@...ira.com]
>Sent: Tuesday, January 25, 2011 9:23 AM
>To: Tantilov, Emil S
>Cc: Kirsher, Jeffrey T; davem@...emloft.net; netdev@...r.kernel.org;
>bphilips@...ell.com; Pieper, Jeffrey E
>Subject: Re: [net-next 08/12] ixgb: convert to new VLAN model
>
>On Sun, Jan 23, 2011 at 4:25 PM, Tantilov, Emil S
><emil.s.tantilov@...el.com> wrote:
>> Jesse Gross wrote:
>>> On Thu, Jan 6, 2011 at 7:29 PM,  <jeffrey.t.kirsher@...el.com> wrote:
>>>> +static int ixgb_set_flags(struct net_device *netdev, u32 data) +{
>>>> +       struct ixgb_adapter *adapter = netdev_priv(netdev); +
>>>> bool need_reset; +       int rc;
>>>> +
>>>> +       /*
>>>> +        * TX vlan insertion does not work per HW design when Rx
>>>> stripping is +        * disabled.  Disable txvlan when rxvlan is
>>>> off. +        */ +       if ((data & ETH_FLAG_RXVLAN) !=
>>>> (netdev->features & NETIF_F_HW_VLAN_RX)) +               data ^=
>>>> ETH_FLAG_TXVLAN;
>>>
>>> Does this really do the right thing?  If the RX vlan setting is
>>> changed, it will do the opposite of what the user requested for TX
>>> vlan?
>>>
>>> So if I start with both on (the default) and turn them both off in one
>>> command (a valid setting), I will get RX off and TX on (an invalid
>>> setting).
>>>
>>> Why not:
>>>
>>> if (!(data & ETH_FLAG_RXVLAN))
>>>         data &= ~ETH_FLAG_TXVLAN;
>>
>> Yeah that works for disabling rxvlan, but what if rxvlan is disabled, and
>the user attempts to enable txvlan? At least our validation argued that we
>should make it work both ways. Perhaps something like the following?
>>
>>        if (!(data & ETH_FLAG_RXVLAN) &&
>>           (netdev->features & NETIF_F_HW_VLAN_TX))
>>                data &= ~ETH_FLAG_TXVLAN;
>>        else if (data & ETH_FLAG_TXVLAN)
>>                data |= ETH_FLAG_RXVLAN;
>
>I think the logic above does what you describe and will always result
>in a consistent state.  Turning dependent features on when needed is a
>little bit inconsistent with the rest of Ethtool (for example, turning
>on TSO when checksum offloading is off will not enable checksum
>offloading, it will produce an error).  However, I know that drivers

That is the reason I asked, as I don't want to keep bouncing the patch back and forth. Personally I like the idea of helping the user and adjusting the flags to something that works rather than a generic error message.

>aren't completely consistent here and the most important part is that
>it enforces valid states, so I don't have a strong opinion.  Ben's
>previous suggestion of Ethtool querying again after the operation and
>reporting any flags that were automatically changed would help a lot
>here.

Sure, but I think a savvy user would always check the result of an ethtool command (ie. `ethtool -K` followed with `ethtool -k`, -A/-a, etc).
`
Added Ben in case he has comments.

Thanks,
Emil
--
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