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 13:29:34 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	"Tantilov, Emil S" <emil.s.tantilov@...el.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.

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