netdev - Re: [PATCH net 3/5] fm10k: Implement ndo_gso

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, 7 Nov 2014 14:35:56 -0800
From:	Joe Stringer <joestringer@...ira.com>
To:	"Vick, Matthew" <matthew.vick@...el.com>
Cc:	"alexander.duyck@...il.com" <alexander.duyck@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Dept-GELinuxNICDev@...gic.com" <Dept-GELinuxNICDev@...gic.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"sathya.perla@...lex.com" <sathya.perla@...lex.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	Linux NICS <Linux-nics@...el.com>,
	"amirv@...lanox.com" <amirv@...lanox.com>,
	"shahed.shaikh@...gic.com" <shahed.shaikh@...gic.com>,
	"therbert@...gle.com" <therbert@...gle.com>,
	"Or Gerlitz" <gerlitz.or@...il.com>
Subject: Re: [PATCH net 3/5] fm10k: Implement ndo_gso_check()

On Friday, November 07, 2014 11:49:38 Vick, Matthew wrote:
> On 11/6/14, 9:05 PM, "Joe Stringer" <joestringer@...ira.com> wrote:
> >Let's merge both discussions into one thread (pick here or there). We
> >have
> >this suggestion or the one which simply checks for tunnels and
> >inner+outer
> >header lengths. Do you have a preference between them?
> 
> Agreed with merging the thread--consider it merged!
> 
> Reflecting on this some more, I prefer the latter option (checking tunnels
> and header lengths). I'm leaning towards pushing the header length check
> into fm10k_tx_encap_offload and then making fm10k_gso_check call that with
> the gso_type. So, it's really the most recent version of the patch you
> proposed:
> 
> static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)
> {
> 	if ((skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) &&
> 	    !fm10k_tx_encap_offload(skb))
> 		return false;
> 
> 	return true;
> }
> 
> 
> plus the header length being checked in fm10k_tx_encap_offload. The only
> nit would be that I'd just return the conditional instead of having
> "return true" or "return false" lines.

OK, that sounds reasonable.

> The tunnel length check really should be there in fm10k_tx_encap_offload
> anyway, so I'll get a patch together for that one.

Thanks.

> >We could introduce an "skb_is_gso_encap()" or similar for this purpose.
> >Checking for SKB_GSO_UDP_TUNNEL or SKB_GSO_GRE is pretty closely tied to
> >what
> >fm10k_tx_encap_offload() checks for though; it might not even make sense
> >to call
> >it if some of the other SKB_GSO_* flags are raised.
> 
> A fair point. On the other hand, we have to check the header length both
> in the GSO and non-GSO cases anyway, so only having the check in
> fm10k_tx_encap_offload and calling it from fm10k_gso_check wouldn't be as
> duplicative. What do you think about that approach?

Sure, I think fm10k_tx_encap_offload() is a good place for the header length 
check. Separately, my question above was regarding the idea of a helper for 
SKB_GSO_{GRE,UDP_TUNNEL}. The only reason it might be useful for the fm10k 
driver is because all encap is checked in the fm10k_tx_encap_offload() function. 
Other drivers don't seem to handle different tunnels together like this though, 
so I'm inclined to stick with the below for now.
                                                                                
static bool fm10k_gso_check(struct sk_buff *skb, struct net_device *dev)        
{                                                                               
        return (!(skb_shinfo(skb)->gso_type &                                   
                  (SKB_GSO_UDP_TUNNEL | SKB_GSO_GRE)) ||                        
                fm10k_tx_encap_offload(skb));                                   
}

Cheers,
Joe
--
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