From: Tvrtko Ursulin <[email protected]>
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
sg_init_table will clear the allocated block so requesting zeroed memory
from the allocator is redundant.
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
From: Tvrtko Ursulin <[email protected]>
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 <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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
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 There is an incosistency between data types used for order and number of
+AF4AXgBeAF4AXgBeAF4AXgBeAF4AXgBe
inconsistency?
+AD4 sg elements in the API.
+AD4
+AD4 Fix it so both are always unsigned int which, in the case of number of
+AD4 elements, matches the underlying struct scatterlist.
Anyway,
Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
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 None of the callers need unsinged long long (they either pass in an int,
+AF4AXgBeAF4AXgBeAF4AXg
unsigned?
+AD4 u32, or size+AF8-t) so it is not required to burden the 32-bit builds with an
+AD4 overspecified length parameter.
Otherwise,
Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
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 sg+AF8-init+AF8-table will clear the allocated block so requesting zeroed memory
+AD4 from the allocator is redundant.
Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
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.
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 We should not use an explicit width u32 for elem+AF8-len but unsinged int to
+AD4 match the underlying type in struct scatterlist.
Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
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 It is necessary to ensure types on both sides of the comparison are of the
+AD4 same width. Otherwise the check overflows sooner than expect due left hand
+AD4 side being an unsigned long length, and the right hand side unsigned int
+AD4 number of elements multiplied by element size.
Reviewed-by: Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4
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 Mostly same fixes and cleanups I've sent earlier in the year, but with some
+AD4 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 +ACI-To+ACI 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.