From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752337AbdKUNEY (ORCPT ); Tue, 21 Nov 2017 08:04:24 -0500 Received: from mx2.suse.de ([195.135.220.15]:41569 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752307AbdKUNEX (ORCPT ); Tue, 21 Nov 2017 08:04:23 -0500 Date: Tue, 21 Nov 2017 14:04:18 +0100 From: Michal Hocko To: Mike Kravetz Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Joonsoo Kim , Vlastimil Babka , Michal Nazarewicz , Mel Gorman , Johannes Weiner , Andrew Morton , stable@vger.kernel.org Subject: Re: [PATCH 1/1] mm/cma: fix alloc_contig_range ret code/potential leak Message-ID: <20171121130418.ythozkqb4phpwth4@dhcp22.suse.cz> References: <20171120193930.23428-1-mike.kravetz@oracle.com> <20171120193930.23428-2-mike.kravetz@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171120193930.23428-2-mike.kravetz@oracle.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 20-11-17 11:39:30, Mike Kravetz wrote: > If the call __alloc_contig_migrate_range() in alloc_contig_range > returns -EBUSY, processing continues so that test_pages_isolated() > is called where there is a tracepoint to identify the busy pages. > However, it is possible for busy pages to become available between > the calls to these two routines. In this case, the range of pages > may be allocated. Unfortunately, the original return code (ret > == -EBUSY) is still set and returned to the caller. Therefore, > the caller believes the pages were not allocated and they are leaked. > > Update the return code with the value from test_pages_isolated(). I find the description in 0/0 much more easier to grasp so I would vote for merging it into the changelog. It took me a while to realize that we are talking about the success path just from staring into the diff which doesn't have a sufficient scope to tell the whole story. Btw. I find embeded description in the diffstat area much more easier to follow for single patches. Having a cover for a single patch sounds more like a distraction. Just my 2c > Fixes: 8ef5849fa8a2 ("mm/cma: always check which page caused allocation failure") > Cc: > Signed-off-by: Mike Kravetz Acked-by: Michal Hocko > --- > mm/page_alloc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 77e4d3c5c57b..3605ca82fd29 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -7632,10 +7632,10 @@ int alloc_contig_range(unsigned long start, unsigned long end, > } > > /* Make sure the range is really isolated. */ > - if (test_pages_isolated(outer_start, end, false)) { > + ret = test_pages_isolated(outer_start, end, false); > + if (ret) { > pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n", > __func__, outer_start, end); > - ret = -EBUSY; > goto done; > } > > -- > 2.13.6 > -- Michal Hocko SUSE Labs