From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vakul Garg Subject: RE: [net-next v5 3/3] net/tls: Remove redundant array allocation. Date: Wed, 1 Aug 2018 13:49:32 +0000 Message-ID: References: <20180719162613.27184-1-vakul.garg@nxp.com> <20180719162613.27184-4-vakul.garg@nxp.com> <20180721.192532.520509909556885779.davem@davemloft.net> <20180723163509.GA91424@davejwatson-mba.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "netdev@vger.kernel.org" , "borisp@mellanox.com" , "aviadye@mellanox.com" , Doron Roberts-Kedes , David Miller To: Dave Watson Return-path: Received: from mail-eopbgr60065.outbound.protection.outlook.com ([40.107.6.65]:22574 "EHLO EUR04-DB3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2389238AbeHAPgK (ORCPT ); Wed, 1 Aug 2018 11:36:10 -0400 In-Reply-To: <20180723163509.GA91424@davejwatson-mba.local> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: > -----Original Message----- > From: Dave Watson [mailto:davejwatson@fb.com] > Sent: Monday, July 23, 2018 10:05 PM > To: David Miller > Cc: Vakul Garg ; netdev@vger.kernel.org; > borisp@mellanox.com; aviadye@mellanox.com; Doron Roberts-Kedes > > Subject: Re: [net-next v5 3/3] net/tls: Remove redundant array allocation= . >=20 > On 07/21/18 07:25 PM, David Miller wrote: > > From: Vakul Garg > > Date: Thu, 19 Jul 2018 21:56:13 +0530 > > > > > In function decrypt_skb(), array allocation in case when sgout is > > > NULL is unnecessary. Instead, local variable sgin_arr[] can be used. > > > > > > Signed-off-by: Vakul Garg > > > > Hmmm... > > > > Dave, can you take a look at this? Do you think there might have been > > a reason you felt that you needed to dynamically allocate the > > scatterlists when you COW and skb and do in-place decryption? > > > > I guess this change is ok, nsg can only get smaller when the SKB is > > COW'd. > > >=20 > > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE); > > > if (!sgout) { > > > nsg =3D skb_cow_data(skb, 0, &unused) + 1; > > > - sgin =3D kmalloc_array(nsg, sizeof(*sgin), sk->sk_all= ocation); > > > sgout =3D sgin; > > > } >=20 > I don't think this patch is safe as-is. sgin_arr is a stack array of siz= e > MAX_SKB_FRAGS (+ overhead), while my read of skb_cow_data is that it > walks the whole chain of skbs from skb->next, and can return any number o= f > segments. Therefore we need to heap allocate. I think I copied the IPSE= C > code here. >=20 > For perf though, we could use the stack array if skb_cow_data returns <= =3D > MAX_SKB_FRAGS. skb_cow_data() is being called only when caller passes sgout=3DNULL (i.e. non-zero copy case). In case of zero-copy, we do not call skb_cow_data() and just assume that MAX_SKB_FRAGS+2 sized scatterlist array sgin_arr[] is sufficient. This assumption could be wrong. So skb_cow_data() should be called unconditionally to determine number of scatterlist entries required for skb. >=20 > This code is slightly confusing though, since we don't heap allocate in t= he > zerocopy case - what happens is that skb_to_sgvec returns -EMSGSIZE, and > we fall back to the non-zerocopy case, and return again to this function, > where we then hit the skb_cow_data path and heap allocate. If skb_to_sgvec return -EMSGSIZE, decrypt_skb() would return failure,=20 resulting in abort of TLS session.