[PATCH net-next] ixgbevf: fix skb->pkt_type checks

[PATCH net-next] ixgbevf: fix skb->pkt_type checks

[Florian Fainelli]

skb->pkt_type is not a bitmask, but contains only value at a time from
the range defined in include/uapi/linux/if_packet.h.

Checking it like if it was a bitmask of values would also cause
PACKET_OTHERHOST, PACKET_LOOPBACK and PACKET_FASTROUTE to be matched by this
check since their lower 2 bits are also set, although that does not fix a real
bug due to how the checks are used, it is still potentially confusing.

This bogus check was introduced in commit 815cccbf ("ixgbe: add setlink,
getlink support to ixgbe and ixgbevf").

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index a6af7b7..c6be010 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -516,8 +516,9 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
 		/* Workaround hardware that can't do proper VEPA multicast
 		 * source pruning.
 		 */
-		if ((skb->pkt_type & (PACKET_BROADCAST | PACKET_MULTICAST)) &&
-		    ether_addr_equal(rx_ring->netdev->dev_addr,
+		if ((skb->pkt_type == PACKET_BROADCAST ||
+		    skb->pkt_type == PACKET_MULTICAST) &&
+		    ether_addr_equal(adapter->netdev->dev_addr,
 				     eth_hdr(skb)->h_source)) {
 			dev_kfree_skb_irq(skb);
 			goto next_desc;
-- 
1.8.3.2

Re: [PATCH net-next] ixgbevf: fix skb->pkt_type checks

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 26 Feb 2014 14:44:00 -0800

> skb->pkt_type is not a bitmask, but contains only value at a time from
> the range defined in include/uapi/linux/if_packet.h.
> 
> Checking it like if it was a bitmask of values would also cause
> PACKET_OTHERHOST, PACKET_LOOPBACK and PACKET_FASTROUTE to be matched by this
> check since their lower 2 bits are also set, although that does not fix a real
> bug due to how the checks are used, it is still potentially confusing.
> 
> This bogus check was introduced in commit 815cccbf ("ixgbe: add setlink,
> getlink support to ixgbe and ixgbevf").
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Intel folks, I assume you will pick this up?

Re: [PATCH net-next] ixgbevf: fix skb->pkt_type checks

[Skidmore, Donald C]

> -----Original Message-----
> From: David Miller [mailto:davem@redhat.com]
> Sent: Thursday, February 27, 2014 1:08 PM
> To: f.fainelli@gmail.com
> Cc: e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org;
> Brandeburg, Jesse
> Subject: Re: [E1000-devel] [PATCH net-next] ixgbevf: fix skb->pkt_type
> checks
> 
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Wed, 26 Feb 2014 14:44:00 -0800
> 
> > skb->pkt_type is not a bitmask, but contains only value at a time from
> > the range defined in include/uapi/linux/if_packet.h.
> >
> > Checking it like if it was a bitmask of values would also cause
> > PACKET_OTHERHOST, PACKET_LOOPBACK and PACKET_FASTROUTE to be
> matched
> > by this check since their lower 2 bits are also set, although that
> > does not fix a real bug due to how the checks are used, it is still potentially
> confusing.
> >
> > This bogus check was introduced in commit 815cccbf ("ixgbe: add
> > setlink, getlink support to ixgbe and ixgbevf").
> >
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Intel folks, I assume you will pick this up?
> 

This patch looks like it fixes an obvious bug on our part.  I'm sure Aaron will get to it soon.

Thanks Florin for pointing this out,
-Don Skidmore <donald.c.skidmore@intel.com>

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH net-next] ixgbevf: fix skb->pkt_type checks

[Aaron Brown]

On Wed, 2014-02-26 at 14:44 -0800, Florian Fainelli wrote:
> skb->pkt_type is not a bitmask, but contains only value at a time from
> the range defined in include/uapi/linux/if_packet.h.
> 
> Checking it like if it was a bitmask of values would also cause
> PACKET_OTHERHOST, PACKET_LOOPBACK and PACKET_FASTROUTE to be matched by this
> check since their lower 2 bits are also set, although that does not fix a real
> bug due to how the checks are used, it is still potentially confusing.
> 
> This bogus check was introduced in commit 815cccbf ("ixgbe: add setlink,
> getlink support to ixgbe and ixgbevf").
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Thanks Florian, I have added this to our internal queue.

Re: [PATCH net-next] ixgbevf: fix skb->pkt_type checks

[Brown, Aaron F]

On Thu, 2014-02-27 at 14:58 -0800, Aaron Brown wrote:
> On Wed, 2014-02-26 at 14:44 -0800, Florian Fainelli wrote:
> > skb->pkt_type is not a bitmask, but contains only value at a time from
> > the range defined in include/uapi/linux/if_packet.h.
> > 
> > Checking it like if it was a bitmask of values would also cause
> > PACKET_OTHERHOST, PACKET_LOOPBACK and PACKET_FASTROUTE to be matched by this
> > check since their lower 2 bits are also set, although that does not fix a real
> > bug due to how the checks are used, it is still potentially confusing.
> > 
> > This bogus check was introduced in commit 815cccbf ("ixgbe: add setlink,
> > getlink support to ixgbe and ixgbevf").
> > 
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Thanks Florian, I have added this to our internal queue.

Unfortunately this introduces a compile error:
----------------------------------------------
/usr/src/kbuild_net-next_community/arch/x86/Makefile:133: CONFIG_X86_X32
enabled but no binutils support
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c: In function
‘ixgbevf_clean_rx_irq’:
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:521: error: ‘adapter’
undeclared (first use in this function)
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:521: error: (Each
undeclared identifier is reported only once
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:521: error: for each
function it appears in.)
make[5]: *** [drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [drivers/net/ethernet/intel/ixgbevf] Error 2
make[3]: *** [drivers/net/ethernet/intel] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [drivers/net/ethernet] Error 2
make[1]: *** [drivers/net] Error 2
make: *** [drivers] Error 2
~                                
----------------------------------------------
Are willing to fix up a v2?

Thanks,
Aaron

Re: [PATCH net-next] ixgbevf: fix skb->pkt_type checks

[Florian Fainelli]

2014-02-28 15:26 GMT-08:00 Brown, Aaron F <aaron.f.brown@intel.com>:
> On Thu, 2014-02-27 at 14:58 -0800, Aaron Brown wrote:
>> On Wed, 2014-02-26 at 14:44 -0800, Florian Fainelli wrote:
>> > skb->pkt_type is not a bitmask, but contains only value at a time from
>> > the range defined in include/uapi/linux/if_packet.h.
>> >
>> > Checking it like if it was a bitmask of values would also cause
>> > PACKET_OTHERHOST, PACKET_LOOPBACK and PACKET_FASTROUTE to be matched by this
>> > check since their lower 2 bits are also set, although that does not fix a real
>> > bug due to how the checks are used, it is still potentially confusing.
>> >
>> > This bogus check was introduced in commit 815cccbf ("ixgbe: add setlink,
>> > getlink support to ixgbe and ixgbevf").
>> >
>> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> > ---
>> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> Thanks Florian, I have added this to our internal queue.
>
> Unfortunately this introduces a compile error:
> ----------------------------------------------
> /usr/src/kbuild_net-next_community/arch/x86/Makefile:133: CONFIG_X86_X32
> enabled but no binutils support
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c: In function
> ‘ixgbevf_clean_rx_irq’:
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:521: error: ‘adapter’
> undeclared (first use in this function)
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:521: error: (Each
> undeclared identifier is reported only once
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c:521: error: for each
> function it appears in.)
> make[5]: *** [drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.o] Error 1
> make[5]: *** Waiting for unfinished jobs....
> make[4]: *** [drivers/net/ethernet/intel/ixgbevf] Error 2
> make[3]: *** [drivers/net/ethernet/intel] Error 2
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [drivers/net/ethernet] Error 2
> make[1]: *** [drivers/net] Error 2
> make: *** [drivers] Error 2
> ~
> ----------------------------------------------
> Are willing to fix up a v2?

Sure, let me re-submit with a fixed version (seems like I rebased
against an earlier version).
-- 
Florian