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
| ||
|
Date: Mon, 15 Jun 2015 13:40:46 +0000 From: Madalin-Cristian Bucur <madalin.bucur@...escale.com> To: Eric Dumazet <eric.dumazet@...il.com> CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet > -----Original Message----- > From: Eric Dumazet [mailto:eric.dumazet@...il.com] > > On Wed, 2015-04-29 at 17:56 +0300, Madalin Bucur wrote: > > This introduces the Freescale Data Path Acceleration Architecture > > (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, > > BMan, PAMU and FMan drivers to deliver Ethernet connectivity on > > the Freescale DPAA QorIQ platforms. > ... > > > + /* We're going to store the skb backpointer at the beginning > > + * of the data buffer, so we need a privately owned skb > > + */ > > + > > + /* Code borrowed from skb_unshare(). */ > > + if (skb_cloned(skb)) { > > + struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC); > > + > > + /* Finally, create a contig FD from this skb */ > > + skb_to_contig_fd(priv, skb, &fd, countptr, &offset); > > + > > + kfree_skb(skb); > > + skb = nskb; > > + /* skb_copy() has now linearized the skbuff. */ > > + } > > + > > You know that TCP packets are clones, right ? > This code is killing performance. > > TCP allows the headers part being modified by a driver if needed. > > You should use skb_header_cloned() instead. Thank you, I'll address this. I plan to do something like this: + if (!nonlinear) { + /* We're going to store the skb backpointer at the beginning + * of the data buffer, so we need a privately owned skb + */ + + /* make sure skb is not shared, skb_cow_head() assumes it's not */ + skb = skb_share_check(skb, GFP_ATOMIC); + if (!skb) + goto enomem; + + /* verify the skb head is not cloned */ + if (skb_cow_head(skb, priv->tx_headroom)) + goto enomem; + + nonlinear = skb_is_nonlinear(skb); + } but I'm not sure the skb_share_check() is required on tx. I'm also a bit puzzled by the aliasing between shared and cloned terms (i.e. skb_unshare() could be named something like skb_unclone(); the skb_share_check() not only checks but also unshares an skb so the already taken skb_unshare() name would probably fit too). Madalin
Powered by blists - more mailing lists