All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Gilbert <dgilbert@interlog.com>
To: linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: martin.petersen@oracle.com, axboe@kernel.dk, bvanassche@acm.org,
	bostroesser@gmail.com
Subject: [PATCH v3 1/4] sgl_alloc_order: remove 4 GiB limit, sgl_free() warning
Date: Mon, 19 Oct 2020 15:19:25 -0400	[thread overview]
Message-ID: <20201019191928.77845-2-dgilbert@interlog.com> (raw)
In-Reply-To: <20201019191928.77845-1-dgilbert@interlog.com>

This patch removes a check done by sgl_alloc_order() before it starts
any allocations. The comment before the removed code says: "Check for
integer overflow" arguably gives a false sense of security. The right
hand side of the expression in the condition is resolved as u32 so
cannot exceed UINT32_MAX (4 GiB) which means 'length' cannot exceed
that amount. If that was the intention then the comment above it
could be dropped and the condition rewritten more clearly as:
     if (length > UINT32_MAX) <<failure path >>;

The author's intention is to use sgl_alloc_order() to replace
vmalloc(unsigned long) for a large allocation (debug ramdisk).
vmalloc has no limit at 4 GiB so its seems unreasonable that:
    sgl_alloc_order(unsigned long long length, ....)
does. sgl_s made with sgl_alloc_order(chainable=false) have equally
sized segments placed in a scatter gather array. That allows O(1)
navigation around a big sgl using some simple integer maths.

Having previously sent a patch to fix a memory leak in
sg_alloc_order() take the opportunity to put a one line comment above
sgl_free()'s declaration that it is not suitable when order > 0 . The
mis-use of sgl_free() when order > 0 was the reason for the memory
leak. The other users of sgl_alloc_order() in the kernel where
checked and found to handle free-ing properly.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 include/linux/scatterlist.h | 1 +
 lib/scatterlist.c           | 3 ---
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 45cf7b69d852..80178afc2a4a 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -302,6 +302,7 @@ struct scatterlist *sgl_alloc(unsigned long 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);
+/* Only use sgl_free() when order is 0 */
 void sgl_free(struct scatterlist *sgl);
 #endif /* CONFIG_SGL_ALLOC */
 
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index c448642e0f78..d5770e7f1030 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -493,9 +493,6 @@ struct scatterlist *sgl_alloc_order(unsigned long long length,
 	u32 elem_len;
 
 	nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
-	/* Check for integer overflow */
-	if (length > (nent << (PAGE_SHIFT + order)))
-		return NULL;
 	nalloc = nent;
 	if (chainable) {
 		/* Check for integer overflow */
-- 
2.25.1


  reply	other threads:[~2020-10-19 19:19 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-19 19:19 [PATCH v3 0/4] scatterlist: add new capabilities Douglas Gilbert
2020-10-19 19:19 ` Douglas Gilbert [this message]
2020-11-03 12:54   ` [PATCH v3 1/4] sgl_alloc_order: remove 4 GiB limit, sgl_free() warning Bodo Stroesser
2020-11-04 18:26     ` Douglas Gilbert
2020-10-19 19:19 ` [PATCH v3 2/4] scatterlist: add sgl_copy_sgl() function Douglas Gilbert
2020-11-03 13:22   ` Bodo Stroesser
2020-11-03 13:26   ` Bodo Stroesser
2020-10-19 19:19 ` [PATCH v3 3/4] scatterlist: add sgl_compare_sgl() function Douglas Gilbert
2020-11-03 13:24   ` Bodo Stroesser
2020-10-19 19:19 ` [PATCH v3 4/4] scatterlist: add sgl_memset() Douglas Gilbert
2020-11-03 13:28   ` Bodo Stroesser

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201019191928.77845-2-dgilbert@interlog.com \
    --to=dgilbert@interlog.com \
    --cc=axboe@kernel.dk \
    --cc=bostroesser@gmail.com \
    --cc=bvanassche@acm.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.