| Message ID | 1288464591-31528-1-git-send-email-jesse@nicira.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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@nicira.com> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > CC: Jesse Brandeburg <jesse.brandeburg@intel.com> > CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com> > --- > drivers/net/ixgb/ixgb_main.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) Thanks Jesse, I have added this to my queue.
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@nicira.com> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > CC: Jesse Brandeburg <jesse.brandeburg@intel.com> > CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.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. Alex has said that he will generate the patches for the alternate fix.
On Fri, Nov 5, 2010 at 12:11 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.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@nicira.com> >> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> CC: Jesse Brandeburg <jesse.brandeburg@intel.com> >> CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.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@nicira.com> > >> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > >> CC: Jesse Brandeburg <jesse.brandeburg@intel.com> > >> CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.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.
>-----Original Message----- >From: Kirsher, Jeffrey T >Sent: Friday, November 05, 2010 1:07 PM >To: Jesse Gross >Cc: David Miller; netdev@vger.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@intel.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@nicira.com> >> >> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> >> CC: Jesse Brandeburg <jesse.brandeburg@intel.com> >> >> CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.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
On Fri, Nov 5, 2010 at 3:30 PM, Duyck, Alexander H <alexander.h.duyck@intel.com> wrote: > > >>-----Original Message----- >>From: Kirsher, Jeffrey T >>Sent: Friday, November 05, 2010 1:07 PM >>To: Jesse Gross >>Cc: David Miller; netdev@vger.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@intel.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@nicira.com> >>> >> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >>> >> CC: Jesse Brandeburg <jesse.brandeburg@intel.com> >>> >> CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 8, 2010 at 11:59 AM, Jesse Gross <jesse@nicira.com> wrote: > On Fri, Nov 5, 2010 at 3:30 PM, Duyck, Alexander H >> 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. Alex, did you get a chance to take a look at this? I was hoping to get the fix for this (and also the patch mentioned originally in this message for ixgb) into 2.6.37. I can also respin it along the lines that you suggested if that would help. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jesse Gross wrote: > On Mon, Nov 8, 2010 at 11:59 AM, Jesse Gross <jesse@nicira.com> wrote: >> On Fri, Nov 5, 2010 at 3:30 PM, Duyck, Alexander H >>> 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. > > Alex, did you get a chance to take a look at this? I was hoping to > get the fix for this (and also the patch mentioned originally in this > message for ixgb) into 2.6.37. I can also respin it along the lines > that you suggested if that would help. Jesse, I am still looking at the ixgb patch. I think the patch is incomplete. Simply removing the adapter->vlgrp check is not enough. According to the specs CTRL0_VME needs to be set in order for the vlan tag insertion/stripping to be enabled. We have a modified patch currently in test and will post it to netdev as soon as we confirm that it works as expected. Thanks, Emil-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>-----Original Message----- >From: Jesse Gross [mailto:jesse@nicira.com] >Sent: Tuesday, November 30, 2010 2:34 PM >To: Duyck, Alexander H >Cc: Kirsher, Jeffrey T; David Miller; netdev@vger.kernel.org; >Brandeburg, Jesse >Subject: Re: [PATCH 1/2] ixgb: Don't check for vlan group on transmit. > >On Mon, Nov 8, 2010 at 11:59 AM, Jesse Gross <jesse@nicira.com> wrote: >> On Fri, Nov 5, 2010 at 3:30 PM, Duyck, Alexander H >>> 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. > >Alex, did you get a chance to take a look at this? I was hoping to >get the fix for this (and also the patch mentioned originally in this >message for ixgb) into 2.6.37. I can also respin it along the lines >that you suggested if that would help. I did, and we are currently testing the patch I have generated. Once it passes testing Jeff will submit it to netdev. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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@nicira.com> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> CC: Jesse Brandeburg <jesse.brandeburg@intel.com> CC: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com> --- drivers/net/ixgb/ixgb_main.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)