netdev - Re: [PATCH 1/2] igb: Allow extra 4 bytes on RX for vlan tags.

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]

Message-ID: <CAKgT0UfaEEvRTSpu-U+0_oj0KnEkyx5hRAwZDiCAAdtY4YhQUQ@mail.gmail.com>
Date:	Wed, 20 Jul 2011 23:35:34 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	jeffrey.t.kirsher@...el.com
Cc:	Ben Greear <greearb@...delatech.com>,
	Jesse Gross <jesse@...ira.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
Subject: Re: [PATCH 1/2] igb: Allow extra 4 bytes on RX for vlan tags.

On Wed, Jul 20, 2011 at 6:21 PM, Jeff Kirsher
<jeffrey.t.kirsher@...el.com> wrote:
> On Wed, 2011-07-20 at 17:27 -0700, Ben Greear wrote:
>> On 07/20/2011 05:18 PM, Jesse Gross wrote:
>> > On Thu, Feb 17, 2011 at 9:28 AM, Ben Greear<greearb@...delatech.com>  wrote:
>> >> On 02/17/2011 03:04 AM, Jeff Kirsher wrote:
>> >>>
>> >>> On Thu, Feb 10, 2011 at 13:59,<greearb@...delatech.com>    wrote:
>> >>>>
>> >>>> From: Ben Greear<greearb@...delatech.com>
>> >>>>
>> >>>> This allows the NIC to receive 1518 byte (not counting
>> >>>> FCS) packets when MTU is 1500, thus allowing 1500 MTU
>> >>>> VLAN frames to be received.  Please note that no VLANs
>> >>>> were actually configured on the NIC...it was just acting
>> >>>> as pass-through device.
>> >>>>
>> >>>> Signed-off-by: Ben Greear<greearb@...delatech.com>
>> >>>> ---
>> >>>> :100644 100644 58c665b... 30c9cc6... M  drivers/net/igb/igb_main.c
>> >>>>   drivers/net/igb/igb_main.c |    5 +++--
>> >>>>   1 files changed, 3 insertions(+), 2 deletions(-)
>> >>>>
>> >>>> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
>> >>>> index 58c665b..30c9cc6 100644
>> >>>> --- a/drivers/net/igb/igb_main.c
>> >>>> +++ b/drivers/net/igb/igb_main.c
>> >>>> @@ -2281,7 +2281,8 @@ static int __devinit igb_sw_init(struct igb_adapter
>> >>>> *adapter)
>> >>>>         adapter->rx_itr_setting = IGB_DEFAULT_ITR;
>> >>>>         adapter->tx_itr_setting = IGB_DEFAULT_ITR;
>> >>>>
>> >>>> -       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
>> >>>> +       adapter->max_frame_size = (netdev->mtu + ETH_HLEN + ETH_FCS_LEN
>> >>>> +                                  + VLAN_HLEN);
>> >>>>         adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
>> >>>>
>> >>>>         spin_lock_init(&adapter->stats64_lock);
>> >>>> @@ -4303,7 +4304,7 @@ static int igb_change_mtu(struct net_device
>> >>>> *netdev, int new_mtu)
>> >>>>   {
>> >>>>         struct igb_adapter *adapter = netdev_priv(netdev);
>> >>>>         struct pci_dev *pdev = adapter->pdev;
>> >>>> -       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
>> >>>> +       int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
>> >>>>         u32 rx_buffer_len, i;
>> >>>>
>> >>>>         if ((new_mtu<    68) || (max_frame>    MAX_JUMBO_FRAME_SIZE)) {
>> >>>
>> >>> While testing this patch, validation found that the patch reduces the
>> >>> maximum mtu size
>> >>> by 4 bytes (reduces it from 9216 to 9212).  This is not a desired side
>> >>> effect of this patch.
>> >>
>> >> You could add handling for that case and have it act as it used to when
>> >> new_mtu is greater than 9212?
>> >>
>> >> I tested e1000e and it worked w/out hacking at 1500 MTU, so maybe
>> >> check how it does it?
>> >
>> > I just wanted to bring this up again to see if any progress had been
>> > made.  We were looking at this driver and trying to figure out the
>> > best way to convert it to use the new vlan model but I'm not familiar
>>
>> I've been watching :)
>>
>> > enough with the hardware to know.  It seems that all of the other
>> > Intel drivers unconditionally add space for the vlan tag to the
>> > receive buffer (and would therefore have similar effects as this
>> > patch), is there something different about this card?
>> >
>> > I believe that Alex was working on something in this area (in the
>> > context of one of my patches from a long time ago) but I'm not sure
>> > what came of that.
>>
>> Truth is, I don't really see why it's a problem to decrease the
>> maximum MTU slightly in order to make it work with VLANs.
>>
>> I'm not sure if there is some way to make it work with VLANs
>> and not decrease the maximum MTU.
>
> This was the reason this did not get accepted.  I was looking into what
> could be done so that we did not decease the maximum MTU, but I got
> side-tracked and have not done anything on it in several months.
>

I can take a look at fixing this most likely tomorrow.  I have some
work planned for igb anyway over the next few days.

Odds are it is just a matter of where the VLAN_HLEN is added.  As I
recall for our drivers the correct spot is in the setting of
rx_buffer_len since that is the area more concerned with maximum
receive frame size versus the mtu section which is more concerned with
the transmit side of things.

Thanks,

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