[PATCH 0/4] badblocks: bugfix and cleanup of _badblocks_check()

[PATCH 0/4] badblocks: bugfix and cleanup of _badblocks_check()

[linan666]

From: Li Nan <linan122@huawei.com>

Li Nan (4):
  badblocks: goto out if find any unacked badblocks in
    _badblocks_check()
  badblocks: optimize _badblocks_check()
  badblocks: fix slab-out-of-bounds in _badblocks_check()
  badblocks: clean up prev_badblocks()

 block/badblocks.c | 48 +++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

-- 
2.39.2

[PATCH 1/4] badblocks: goto out if find any unacked badblocks in _badblocks_check()

[linan666]

From: Li Nan <linan122@huawei.com>

In _badblocks_check(), after finding any unacked badblocks, the return
value, 'first_bad' and 'bad_sectors' will not be changed anymore, and
there is no need to check other ranges. So goto out directly after
finding unacked badblocks.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index fc92d4e18aa3..ebf17a54851a 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -1274,7 +1274,7 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 			    sector_t *first_bad, int *bad_sectors)
 {
-	int unacked_badblocks, acked_badblocks;
+	int acked_badblocks;
 	int prev = -1, hint = -1, set = 0;
 	struct badblocks_context bad;
 	unsigned int seq;
@@ -1297,7 +1297,6 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	seq = read_seqbegin(&bb->lock);
 
 	p = bb->page;
-	unacked_badblocks = 0;
 	acked_badblocks = 0;
 
 re_check:
@@ -1318,21 +1317,24 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	}
 
 	if (overlap_front(bb, prev, &bad)) {
-		if (BB_ACK(p[prev]))
+		if (set == 0) {
+			*first_bad = BB_OFFSET(p[prev]);
+			*bad_sectors = BB_LEN(p[prev]);
+			set = 1;
+		}
+
+		if (BB_ACK(p[prev])) {
 			acked_badblocks++;
-		else
-			unacked_badblocks++;
+		} else {
+			rv = -1;
+			goto out;
+		}
 
 		if (BB_END(p[prev]) >= (s + sectors))
 			len = sectors;
 		else
 			len = BB_END(p[prev]) - s;
 
-		if (set == 0) {
-			*first_bad = BB_OFFSET(p[prev]);
-			*bad_sectors = BB_LEN(p[prev]);
-			set = 1;
-		}
 		goto update_sectors;
 	}
 
@@ -1355,13 +1357,12 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 
 	WARN_ON(sectors < 0);
 
-	if (unacked_badblocks > 0)
-		rv = -1;
-	else if (acked_badblocks > 0)
+	if (acked_badblocks > 0)
 		rv = 1;
 	else
 		rv = 0;
 
+out:
 	if (read_seqretry(&bb->lock, seq))
 		goto retry;
 
-- 
2.39.2

[PATCH 2/4] badblocks: optimize _badblocks_check()

[linan666]

From: Li Nan <linan122@huawei.com>

Check badblocks_empty earlier, and goto out if check range starts after
all badblocks or badblocks is empty since no badblocks intersect with
check range.

Clean up redundant check '(prev + 1) < bb->count'. If it is true, it
will enter the earlier branch.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index ebf17a54851a..054d05b93641 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -1296,6 +1296,11 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 retry:
 	seq = read_seqbegin(&bb->lock);
 
+	if (badblocks_empty(bb)) {
+		len = sectors;
+		goto out;
+	}
+
 	p = bb->page;
 	acked_badblocks = 0;
 
@@ -1303,17 +1308,12 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	bad.start = s;
 	bad.len = sectors;
 
-	if (badblocks_empty(bb)) {
-		len = sectors;
-		goto update_sectors;
-	}
-
 	prev = prev_badblocks(bb, &bad, hint);
 
 	/* start after all badblocks */
 	if ((prev + 1) >= bb->count && !overlap_front(bb, prev, &bad)) {
 		len = sectors;
-		goto update_sectors;
+		goto out;
 	}
 
 	if (overlap_front(bb, prev, &bad)) {
@@ -1339,7 +1339,7 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	}
 
 	/* Not front overlap, but behind overlap */
-	if ((prev + 1) < bb->count && overlap_behind(bb, &bad, prev + 1)) {
+	if (overlap_behind(bb, &bad, prev + 1)) {
 		len = BB_OFFSET(p[prev + 1]) - bad.start;
 		hint = prev + 1;
 		goto update_sectors;
-- 
2.39.2

[PATCH 3/4] badblocks: fix slab-out-of-bounds in _badblocks_check()

[linan666]

From: Li Nan <linan122@huawei.com>

prev_badblocks() will return -1 if checked range start < p[0]. Accessing
p[-1] will cause bug as below:

  BUG: KASAN: slab-out-of-bounds in badblocks_check+0x2c4

Fix it by checking 'prev' before accessing badblocks->page.

Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 054d05b93641..71a3e43351da 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -1316,7 +1316,7 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 		goto out;
 	}
 
-	if (overlap_front(bb, prev, &bad)) {
+	if (prev >= 0 && overlap_front(bb, prev, &bad)) {
 		if (set == 0) {
 			*first_bad = BB_OFFSET(p[prev]);
 			*bad_sectors = BB_LEN(p[prev]);
-- 
2.39.2

[PATCH 4/4] badblocks: clean up prev_badblocks()

[linan666]

From: Li Nan <linan122@huawei.com>

The offset of 'lo' must <= ‘s' after biset. clean up redundant check.
And replace the check of badblocks->count with badblocks_empty().

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 71a3e43351da..88ed13897443 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -486,7 +486,7 @@ static int prev_badblocks(struct badblocks *bb, struct badblocks_context *bad,
 	int lo, hi;
 	u64 *p;
 
-	if (!bb->count)
+	if (badblocks_empty(bb))
 		goto out;
 
 	if (hint >= 0) {
@@ -521,8 +521,7 @@ static int prev_badblocks(struct badblocks *bb, struct badblocks_context *bad,
 			hi = mid;
 	}
 
-	if (BB_OFFSET(p[lo]) <= s)
-		ret = lo;
+	ret = lo;
 out:
 	return ret;
 }
-- 
2.39.2

Re: [PATCH 0/4] badblocks: bugfix and cleanup of _badblocks_check()

[Ira Weiny]

linan666@ wrote:
> From: Li Nan <linan122@huawei.com>
> 
> Li Nan (4):
>   badblocks: goto out if find any unacked badblocks in
>     _badblocks_check()
>   badblocks: optimize _badblocks_check()
>   badblocks: fix slab-out-of-bounds in _badblocks_check()
>   badblocks: clean up prev_badblocks()
> 
>  block/badblocks.c | 48 +++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> -- 
> 2.39.2
> 

Thanks for the series!  Unfortunately I'm still seeing some failures with
this series.

Coly's test patch[1] fixed all my test failures.  Right off the top I'm
not seeing what you missed that he seemed to catch.

Ira

[1] https://lore.kernel.org/all/nhza4xsnbmcmka7463jxgmdvb27pqvbvcuzs7xp4vzpqlo262d@dp7laevqtaka/