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]

Message-ID: <AANLkTin5YMqpq0h-2HQ8XgaheYYDeMUFv8Qo6fZdWhxB@mail.gmail.com>
Date:	Mon, 8 Nov 2010 11:59:07 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
Cc:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	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.

On Fri, Nov 5, 2010 at 3:30 PM, Duyck, Alexander H
<alexander.h.duyck@...el.com> wrote:
>
>
>>-----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.
>

OK, that makes more sense.  It seems that the other drivers already
account for this, so they should be fine.

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

I happy to let you take care of it - obviously you know the
driver/hardware much better than I.
--
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