Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1538580700.205649.10.camel@acm.org>
Subject: Re: [PATCH 4/6] lib/scatterlist: Do not leak pages when high-order
 allocation fails
From:   Bart Van Assche <bvanassche@acm.org>
To:     Tvrtko Ursulin <tursulin@ursulin.net>, linux-kernel@vger.kernel.org
Cc:     tvrtko.ursulin@linux.intel.com,
        Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
        Bart Van Assche <bart.vanassche@wdc.com>,
        Hannes Reinecke <hare@suse.com>,
        Johannes Thumshirn <jthumshirn@suse.de>,
        Jens Axboe <axboe@kernel.dk>
Date:   Wed, 03 Oct 2018 08:31:40 -0700
In-Reply-To: <20180926141625.17727-5-tvrtko.ursulin@linux.intel.com>
References: <20180926141625.17727-1-tvrtko.ursulin@linux.intel.com>
         <20180926141625.17727-5-tvrtko.ursulin@linux.intel.com>
Content-Type: text/plain; charset="UTF-7"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 2018-09-26 at 15:16 +-0100, Tvrtko Ursulin wrote:
+AD4 From: Tvrtko Ursulin +ADw-tvrtko.ursulin+AEA-intel.com+AD4
+AD4 
+AD4 If a higher-order allocation fails, the existing abort and cleanup path
+AD4 would consider all segments allocated so far as 0-order page allocations
+AD4 and would therefore leak memory.
+AD4 
+AD4 Fix this by cleaning up using sgl+AF8-free+AF8-n+AF8-order which allows the correct
+AD4 page order to be passed in.
+AD4 
+AD4 Signed-off-by: Tvrtko Ursulin +ADw-tvrtko.ursulin+AEA-intel.com+AD4
+AD4 Cc: Bart Van Assche +ADw-bart.vanassche+AEA-wdc.com+AD4
+AD4 Cc: Hannes Reinecke +ADw-hare+AEA-suse.com+AD4
+AD4 Cc: Johannes Thumshirn +ADw-jthumshirn+AEA-suse.de+AD4
+AD4 Cc: Jens Axboe +ADw-axboe+AEA-kernel.dk+AD4
+AD4 ---
+AD4  lib/scatterlist.c +AHw 6 +-+-+-+---
+AD4  1 file changed, 4 insertions(+-), 2 deletions(-)
+AD4 
+AD4 diff --git a/lib/scatterlist.c b/lib/scatterlist.c
+AD4 index 3cc01cd82242..0caed79d7291 100644
+AD4 --- a/lib/scatterlist.c
+AD4 +-+-+- b/lib/scatterlist.c
+AD4 +AEAAQA -481,7 +-481,7 +AEAAQA struct scatterlist +ACo-sgl+AF8-alloc+AF8-order(unsigned long length, unsigned int order,
+AD4  +AHs
+AD4  	struct scatterlist +ACo-sgl, +ACo-sg+ADs
+AD4  	struct page +ACo-page+ADs
+AD4 -	unsigned int nent, nalloc+ADs
+AD4 +-	unsigned int nent, nalloc, i+ADs
+AD4  	u32 elem+AF8-len+ADs
+AD4  
+AD4  	nent +AD0 round+AF8-up(length, PAGE+AF8-SIZE +ADwAPA order) +AD4APg (PAGE+AF8-SHIFT +- order)+ADs
+AD4 +AEAAQA -501,17 +-501,19 +AEAAQA struct scatterlist +ACo-sgl+AF8-alloc+AF8-order(unsigned long length, unsigned int order,
+AD4  
+AD4  	sg+AF8-init+AF8-table(sgl, nalloc)+ADs
+AD4  	sg +AD0 sgl+ADs
+AD4 +-	i +AD0 0+ADs
+AD4  	while (length) +AHs
+AD4  		elem+AF8-len +AD0 min+AF8-t(u64, length, PAGE+AF8-SIZE +ADwAPA order)+ADs
+AD4  		page +AD0 alloc+AF8-pages(gfp, order)+ADs
+AD4  		if (+ACE-page) +AHs
+AD4 -			sgl+AF8-free(sgl)+ADs
+AD4 +-			sgl+AF8-free+AF8-n+AF8-order(sgl, i, order)+ADs
+AD4  			return NULL+ADs
+AD4  		+AH0
+AD4  
+AD4  		sg+AF8-set+AF8-page(sg, page, elem+AF8-len, 0)+ADs
+AD4  		length -+AD0 elem+AF8-len+ADs
+AD4  		sg +AD0 sg+AF8-next(sg)+ADs
+AD4 +-		i+-+-+ADs
+AD4  	+AH0
+AD4  	WARN+AF8-ONCE(length, +ACI-length +AD0 +ACU-ld+AFw-n+ACI, length)+ADs
+AD4  	if (nent+AF8-p)

sg+AF8-init+AF8-table() clears all page pointers and sgl+AF8-free+AF8-n+AF8-order() can handle
elements of which the page pointer is zero. So I think if
sgl+AF8-free+AF8-n+AF8-order(sgl, i, order) would be changed into sgl+AF8-free+AF8-n+AF8-order(sgl,
UINT+AF8-MAX, order) that the variable 'i' can be left out.

Bart.