netdev - RE: [PATCH 1/2] ixgb: Don't check for vlan group on transmit.

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:	Fri, 5 Nov 2010 15:30:31 -0700
From:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	Jesse Gross <jesse@...ira.com>
CC:	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>
Subject: RE: [PATCH 1/2] ixgb: Don't check for vlan group on transmit.



>-----Original Message-----
>From: Kirsher, Jeffrey T
>Sent: Friday, November 05, 2010 1:07 PM
>To: Jesse Gross
>Cc: David Miller; netdev@...r.kernel.org; Brandeburg, Jesse; Duyck,
>Alexander H
>Subject: Re: [PATCH 1/2] ixgb: Don't check for vlan group on transmit.
>
>On Fri, 2010-11-05 at 12:56 -0700, Jesse Gross wrote:
>> On Fri, Nov 5, 2010 at 12:11 PM, Jeff Kirsher
>> <jeffrey.t.kirsher@...el.com> wrote:
>> > On Sat, 2010-10-30 at 11:49 -0700, Jesse Gross wrote:
>> >> On transmit, the ixgb driver will only use vlan acceleration if a
>> >> vlan group is configured.  This can lead to tags getting dropped
>> >> when bridging because the networking core assumes that a driver
>> >> that claims vlan acceleration support can do it at all times.
>This
>> >> change should have been part of commit eab6d18d "vlan: Don't
>check for
>> >> vlan group before vlan_tx_tag_present." but was missed.
>> >>
>> >> Signed-off-by: Jesse Gross <jesse@...ira.com>
>> >> CC: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>> >> CC: Jesse Brandeburg <jesse.brandeburg@...el.com>
>> >> CC: PJ Waskiewicz <peter.p.waskiewicz.jr@...el.com>
>> >> ---
>> >>  drivers/net/ixgb/ixgb_main.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/drivers/net/ixgb/ixgb_main.c
>b/drivers/net/ixgb/ixgb_main.c
>> >> index caa8192..d18194e 100644
>> >> --- a/drivers/net/ixgb/ixgb_main.c
>> >> +++ b/drivers/net/ixgb/ixgb_main.c
>> >> @@ -1498,7 +1498,7 @@ ixgb_xmit_frame(struct sk_buff *skb, struct
>net_device *netdev)
>> >>                       DESC_NEEDED)))
>> >>               return NETDEV_TX_BUSY;
>> >>
>> >> -     if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
>> >> +     if (vlan_tx_tag_present(skb)) {
>> >>               tx_flags |= IXGB_TX_FLAGS_VLAN;
>> >>               vlan_id = vlan_tx_tag_get(skb);
>> >>       }
>> >
>> > After further review, NAK because this will cause a bug.  With
>this
>> > patch it would be possible to overrun the buffers, so the correct
>fix is
>> > to increase max_frame_size by VLAN_TAG_SIZE in
>ixgb/igb_change_mtu.
>>
>> Hmm, I didn't see any other place where it made changes to the
>> handling of packets on transmit if a vlan group is configured.
>Maybe
>> the buffer is extended when a group is registered and stripping is
>> enabled?
>>
>> In any case, you might want to check the other Intel drivers for
>> similar problems.  I did a pass and made a mass conversion of this
>> type a little while ago.  Those changes have already been merged, I
>> just missed this one by accident.
>
>I will get with Alex and review the other Intel drivers, thanks Jesse.

Just to make things clear.  The ixgb patch is fine.  There isn't anything wrong with it.

The patch with the bug is the other patch, "2/2 igb: Don't depend on VLAN group for receive size".  The problem is it was updating the RLPML register, but not updating the buffer sizes as such there were a few cases where we could receive a buffer larger the SKB head room.  The bug itself probably won't come up very often since there are only a couple of very specific MTU sizes where it will be an issue.

The quick fix for your patch is to move the addition of VLAN_TAG_SIZE to the max_frame in igb_change_mtu instead of in the set_rlpml call.  Otherwise I will see about submitting an updated patch in the next few days.

Thanks,

Alex