[PATCH 0/6] lib/scatterlist: sgl API fixes and cleanups

[PATCH 0/6] lib/scatterlist: sgl API fixes and cleanups

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Mostly same fixes and cleanups I've sent earlier in the year, but with some
patches dropped and some split into smaller ones as per request.

Tvrtko Ursulin (6):
  lib/scatterlist: Use natural long for sgl_alloc(_order) length
    parameter
  lib/scatterlist: Use consistent types in sgl API
  lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order
  lib/scatterlist: Do not leak pages when high-order allocation fails
  lib/scatterlist: Use appropriate type for elem_len in sgl_alloc_order
  lib/scatterlist: Fix overflow check in sgl_alloc_order

 include/linux/scatterlist.h | 13 +++++++------
 lib/scatterlist.c           | 33 +++++++++++++++++----------------
 2 files changed, 24 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/6] lib/scatterlist: Use natural long for sgl_alloc(_order) length parameter

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

None of the callers need unsinged long long (they either pass in an int,
u32, or size_t) so it is not required to burden the 32-bit builds with an
overspecified length parameter.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
 include/linux/scatterlist.h |  8 ++++----
 lib/scatterlist.c           | 10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 093aa57120b0..bdede25901b5 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -280,10 +280,10 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
 			      unsigned long size, gfp_t gfp_mask);
 
 #ifdef CONFIG_SGL_ALLOC
-struct scatterlist *sgl_alloc_order(unsigned long long length,
-				    unsigned int order, bool chainable,
-				    gfp_t gfp, unsigned int *nent_p);
-struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
+				    bool chainable, gfp_t gfp,
+				    unsigned int *nent_p);
+struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
 			      unsigned int *nent_p);
 void sgl_free_n_order(struct scatterlist *sgl, int nents, int order);
 void sgl_free_order(struct scatterlist *sgl, int order);
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 7c6096a71704..014018f90e28 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -475,9 +475,9 @@ EXPORT_SYMBOL(sg_alloc_table_from_pages);
  *
  * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
  */
-struct scatterlist *sgl_alloc_order(unsigned long long length,
-				    unsigned int order, bool chainable,
-				    gfp_t gfp, unsigned int *nent_p)
+struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
+				    bool chainable, gfp_t gfp,
+				    unsigned int *nent_p)
 {
 	struct scatterlist *sgl, *sg;
 	struct page *page;
@@ -514,7 +514,7 @@ struct scatterlist *sgl_alloc_order(unsigned long long length,
 		length -= elem_len;
 		sg = sg_next(sg);
 	}
-	WARN_ONCE(length, "length = %lld\n", length);
+	WARN_ONCE(length, "length = %ld\n", length);
 	if (nent_p)
 		*nent_p = nent;
 	return sgl;
@@ -529,7 +529,7 @@ EXPORT_SYMBOL(sgl_alloc_order);
  *
  * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
  */
-struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
 			      unsigned int *nent_p)
 {
 	return sgl_alloc_order(length, 0, false, gfp, nent_p);
-- 
2.17.1

Re: [PATCH 1/6] lib/scatterlist: Use natural long for sgl_alloc(_order) length parameter

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> None of the callers need unsinged long long (they either pass in an int,
                           ^^^^^^^^
                           unsigned?
> u32, or size_t) so it is not required to burden the 32-bit builds with an
> overspecified length parameter.

Otherwise,

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 2/6] lib/scatterlist: Use consistent types in sgl API

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

There is an incosistency between data types used for order and number of
sg elements in the API.

Fix it so both are always unsigned int which, in the case of number of
elements, matches the underlying struct scatterlist.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
 include/linux/scatterlist.h | 5 +++--
 lib/scatterlist.c           | 9 +++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index bdede25901b5..f6cf4d7c9a69 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -285,8 +285,9 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 				    unsigned int *nent_p);
 struct scatterlist *sgl_alloc(unsigned long length, gfp_t gfp,
 			      unsigned int *nent_p);
-void sgl_free_n_order(struct scatterlist *sgl, int nents, int order);
-void sgl_free_order(struct scatterlist *sgl, int order);
+void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
+		      unsigned int order);
+void sgl_free_order(struct scatterlist *sgl, unsigned int order);
 void sgl_free(struct scatterlist *sgl);
 #endif /* CONFIG_SGL_ALLOC */
 
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 014018f90e28..23e53dce897d 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -549,11 +549,12 @@ EXPORT_SYMBOL(sgl_alloc);
  * - All pages in a chained scatterlist can be freed at once by setting @nents
  *   to a high number.
  */
-void sgl_free_n_order(struct scatterlist *sgl, int nents, int order)
+void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
+		      unsigned int order)
 {
 	struct scatterlist *sg;
 	struct page *page;
-	int i;
+	unsigned int i;
 
 	for_each_sg(sgl, sg, nents, i) {
 		if (!sg)
@@ -571,9 +572,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
  * @sgl: Scatterlist with one or more elements
  * @order: Second argument for __free_pages()
  */
-void sgl_free_order(struct scatterlist *sgl, int order)
+void sgl_free_order(struct scatterlist *sgl, unsigned int order)
 {
-	sgl_free_n_order(sgl, INT_MAX, order);
+	sgl_free_n_order(sgl, UINT_MAX, order);
 }
 EXPORT_SYMBOL(sgl_free_order);
 
-- 
2.17.1

Re: [PATCH 2/6] lib/scatterlist: Use consistent types in sgl API

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> There is an incosistency between data types used for order and number of
              ^^^^^^^^^^^^
              inconsistency?
> sg elements in the API.
> 
> Fix it so both are always unsigned int which, in the case of number of
> elements, matches the underlying struct scatterlist.

Anyway,

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 3/6] lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

sg_init_table will clear the allocated block so requesting zeroed memory
from the allocator is redundant.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
 lib/scatterlist.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 23e53dce897d..3cc01cd82242 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -495,8 +495,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 			return NULL;
 		nalloc++;
 	}
-	sgl = kmalloc_array(nalloc, sizeof(struct scatterlist),
-			    (gfp & ~GFP_DMA) | __GFP_ZERO);
+	sgl = kmalloc_array(nalloc, sizeof(struct scatterlist), gfp & ~GFP_DMA);
 	if (!sgl)
 		return NULL;
 
-- 
2.17.1

Re: [PATCH 3/6] lib/scatterlist: Skip requesting zeroed allocations in sgl_alloc_order

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> sg_init_table will clear the allocated block so requesting zeroed memory
> from the allocator is redundant.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 4/6] lib/scatterlist: Do not leak pages when high-order allocation fails

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

If a higher-order allocation fails, the existing abort and cleanup path
would consider all segments allocated so far as 0-order page allocations
and would therefore leak memory.

Fix this by cleaning up using sgl_free_n_order which allows the correct
page order to be passed in.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
 lib/scatterlist.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 3cc01cd82242..0caed79d7291 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -481,7 +481,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 {
 	struct scatterlist *sgl, *sg;
 	struct page *page;
-	unsigned int nent, nalloc;
+	unsigned int nent, nalloc, i;
 	u32 elem_len;
 
 	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
@@ -501,17 +501,19 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 
 	sg_init_table(sgl, nalloc);
 	sg = sgl;
+	i = 0;
 	while (length) {
 		elem_len = min_t(u64, length, PAGE_SIZE << order);
 		page = alloc_pages(gfp, order);
 		if (!page) {
-			sgl_free(sgl);
+			sgl_free_n_order(sgl, i, order);
 			return NULL;
 		}
 
 		sg_set_page(sg, page, elem_len, 0);
 		length -= elem_len;
 		sg = sg_next(sg);
+		i++;
 	}
 	WARN_ONCE(length, "length = %ld\n", length);
 	if (nent_p)
-- 
2.17.1

Re: [PATCH 4/6] lib/scatterlist: Do not leak pages when high-order allocation fails

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> If a higher-order allocation fails, the existing abort and cleanup path
> would consider all segments allocated so far as 0-order page allocations
> and would therefore leak memory.
> 
> Fix this by cleaning up using sgl_free_n_order which allows the correct
> page order to be passed in.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
>  lib/scatterlist.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index 3cc01cd82242..0caed79d7291 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -481,7 +481,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
>  {
>  	struct scatterlist *sgl, *sg;
>  	struct page *page;
> -	unsigned int nent, nalloc;
> +	unsigned int nent, nalloc, i;
>  	u32 elem_len;
>  
>  	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
> @@ -501,17 +501,19 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
>  
>  	sg_init_table(sgl, nalloc);
>  	sg = sgl;
> +	i = 0;
>  	while (length) {
>  		elem_len = min_t(u64, length, PAGE_SIZE << order);
>  		page = alloc_pages(gfp, order);
>  		if (!page) {
> -			sgl_free(sgl);
> +			sgl_free_n_order(sgl, i, order);
>  			return NULL;
>  		}
>  
>  		sg_set_page(sg, page, elem_len, 0);
>  		length -= elem_len;
>  		sg = sg_next(sg);
> +		i++;
>  	}
>  	WARN_ONCE(length, "length = %ld\n", length);
>  	if (nent_p)

sg_init_table() clears all page pointers and sgl_free_n_order() can handle
elements of which the page pointer is zero. So I think if
sgl_free_n_order(sgl, i, order) would be changed into sgl_free_n_order(sgl,
UINT_MAX, order) that the variable 'i' can be left out.

Bart.

[PATCH 5/6] lib/scatterlist: Use appropriate type for elem_len in sgl_alloc_order

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We should not use an explicit width u32 for elem_len but unsinged int to
match the underlying type in struct scatterlist.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
 lib/scatterlist.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 0caed79d7291..581a2e91e515 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -481,8 +481,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 {
 	struct scatterlist *sgl, *sg;
 	struct page *page;
-	unsigned int nent, nalloc, i;
-	u32 elem_len;
+	unsigned int nent, nalloc, elem_len, i;
 
 	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
 	/* Check for integer overflow */
@@ -503,7 +502,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 	sg = sgl;
 	i = 0;
 	while (length) {
-		elem_len = min_t(u64, length, PAGE_SIZE << order);
+		elem_len = min_t(unsigned long, length, PAGE_SIZE << order);
 		page = alloc_pages(gfp, order);
 		if (!page) {
 			sgl_free_n_order(sgl, i, order);
-- 
2.17.1

Re: [PATCH 5/6] lib/scatterlist: Use appropriate type for elem_len in sgl_alloc_order

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We should not use an explicit width u32 for elem_len but unsinged int to
> match the underlying type in struct scatterlist.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 6/6] lib/scatterlist: Fix overflow check in sgl_alloc_order

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It is necessary to ensure types on both sides of the comparison are of the
same width. Otherwise the check overflows sooner than expect due left hand
side being an unsigned long length, and the right hand side unsigned int
number of elements multiplied by element size.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
 lib/scatterlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 581a2e91e515..c87243d46f10 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -485,7 +485,7 @@ struct scatterlist *sgl_alloc_order(unsigned long length, unsigned int order,
 
 	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
 	/* Check for integer overflow */
-	if (length > (nent << (PAGE_SHIFT + order)))
+	if (length > ((unsigned long)nent << (PAGE_SHIFT + order)))
 		return NULL;
 	nalloc = nent;
 	if (chainable) {
-- 
2.17.1

Re: [PATCH 6/6] lib/scatterlist: Fix overflow check in sgl_alloc_order

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> It is necessary to ensure types on both sides of the comparison are of the
> same width. Otherwise the check overflows sooner than expect due left hand
> side being an unsigned long length, and the right hand side unsigned int
> number of elements multiplied by element size.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 0/6] lib/scatterlist: sgl API fixes and cleanups

[Bart Van Assche]

On Wed, 2018-09-26 at 15:16 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Mostly same fixes and cleanups I've sent earlier in the year, but with some
> patches dropped and some split into smaller ones as per request.

I think in order for a patch series to get noticed that the maintainer the
series is sent to should appear in the "To" field of the e-mail. If you
resend this patch series, please Cc the linux-block mailing list and please
also use my current e-mail address. I only noticed this patch series today
because my old e-mail address occurs in the Cc list of this patch series.

Thanks,

Bart.