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:	Wed, 15 Dec 2010 11:09:37 -0700
From:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
To:	Jesse Gross <jesse@...ira.com>
CC:	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>,
	Ben Hutchings <bhutchings@...arflare.com>
Subject: RE: [PATCH] ixgb: Convert to new vlan model.

Jesse Gross wrote:
> On Tue, Dec 14, 2010 at 11:15 AM, Ben Hutchings
> <bhutchings@...arflare.com> wrote:
>> On Tue, 2010-12-14 at 12:08 -0700, Tantilov, Emil S wrote:
>>> 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.
>> 
>> Then it's *not* a valid setting for your hardware/driver.
> 
> Ben, I agree that limiting the settings to what is actually supported
> is conceptually cleaner but in practice it's not very intuitive.  If
> you try to turn something off and the response is that it's invalid,
> most people are going to assume that you just can't do it.  This is
> especially true since you actually can't turn these settings off in
> most drivers.
> 
> There's a precedent for this type of thing: turn off TX checksum
> offloading and watch scatter/gather and TSO be automatically disabled
> as well.  It makes sense - the user requested a change, we do what is
> necessary to make that happen without requiring them to understand why
> these features are interrelated.
> 
> Emil, I realized afterwards that, as you pointed out, TX vlan
> offloading can be disabled without requiring RX offloading to be
> disabled.  Feel free to make the modification yourself or I can
> resubmit, whichever is easier.

If it's OK with you I can make whatever changes are needed since I need
to test this first, so I don't want to go back and forth in case we find
other issues in testing.

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