From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Lavigne, Jamie" Subject: Re: [PATCH v2] Correctly handle malloc_elem resize with padding Date: Thu, 8 Jun 2017 19:07:42 +0000 Message-ID: <1496948862726.3852@amazon.com> References: <1496189340-27813-1-git-send-email-lavignen@amazon.com> <1496189818-2307-1-git-send-email-lavignen@amazon.com>, <3b80fa21-3d3e-73ae-235b-7581db5e8670@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Sergio Gonzalez Monroy , "dev@dpdk.org" Return-path: Received: from smtp-fw-2101.amazon.com (smtp-fw-2101.amazon.com [72.21.196.25]) by dpdk.org (Postfix) with ESMTP id C18032BA2 for ; Thu, 8 Jun 2017 21:07:53 +0200 (CEST) In-Reply-To: <3b80fa21-3d3e-73ae-235b-7581db5e8670@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Sergio,=0A= =0A= > Hi Jamie, =0A= > =0A= > On 31/05/2017 01:16, Jamie Lavigne wrote: =0A= > > Currently when a malloc_elem is split after resizing, any padding =0A= > > present in the elem is ignored. This causes the resized elem to be too= =0A= > > small when padding is present, and user data can overwrite the beginnin= g =0A= > > of the following malloc_elem. =0A= > > =0A= > > Solve this by including the size of the padding when computing where to= =0A= > > split the malloc_elem. =0A= > =0A= > Nice catch! =0A= > =0A= > Could you please rework commit format a bit: =0A= > - Add 'mem:' as prefix in your patch title =0A= > - I would mention in the title that this is a fix =0A= > - Provide 'Fixes' line in commit message =0A= =0A= Updated.=0A= =0A= > =0A= > > Signed-off-by: Jamie Lavigne =0A= > > --- =0A= > > lib/librte_eal/common/malloc_elem.c | 6 ++++-- =0A= > > 1 file changed, 4 insertions(+), 2 deletions(-) =0A= > > =0A= > > diff --git a/lib/librte_eal/common/malloc_elem.c b/lib/librte_eal/commo= n/malloc_elem.c =0A= > > index 42568e1..8766fa8 100644 =0A= > > --- a/lib/librte_eal/common/malloc_elem.c =0A= > > +++ b/lib/librte_eal/common/malloc_elem.c =0A= > > @@ -333,9 +333,11 @@ malloc_elem_resize(struct malloc_elem *elem, size_= t size) =0A= > > elem_free_list_remove(next); =0A= > > join_elem(elem, next); =0A= > > =0A= > > - if (elem->size - new_size >=3D MIN_DATA_SIZE + MALLOC_ELEM_OVERHE= AD){ =0A= > > + const size_t new_total_size =3D new_size + elem->pad; =0A= > > + =0A= > > + if (elem->size - new_total_size >=3D MIN_DATA_SIZE + MALLOC_ELEM_= OVERHEAD) { =0A= > > /* now we have a big block together. Lets cut it down a b= it, by splitting */ =0A= > > - struct malloc_elem *split_pt =3D RTE_PTR_ADD(elem, new_si= ze); =0A= > > + struct malloc_elem *split_pt =3D RTE_PTR_ADD(elem, new_to= tal_size); =0A= > > split_pt =3D RTE_PTR_ALIGN_CEIL(split_pt, RTE_CACHE_LINE_= SIZE); =0A= > > split_elem(elem, split_pt); =0A= > > malloc_elem_free_list_insert(split_pt); =0A= > =0A= > This indeed fixes the issue you have mentioned. I was thinking of the =0A= > following fix instead: =0A= > - Add elem->pad to new_size =0A= > - Remove current_size var and instead use elem->size =0A= > =0A= > I think those changes should have the same result while removing a =0A= > couple of vars from the function, which I hope would be easier to read. = =0A= > =0A= > What do you think? =0A= =0A= I like this. It looks equivalent to my solution, but simpler. I will post= an updated patch.=0A= =0A= Jamie=0A=