md:Avoid write invalid address if read_seqretry returned true.
PATCH 1/2 md:Avoid write invalid address if read_seqretry returned true.
If read_seqretry returned true and bbp was changed, it will write invalid address which can cause some serious problem.
This bug was introduced by commit v3.0-rc7-130-g2699b67. So fix is suitable for 3.0.y thru 3.6.y.
--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/md/md.c +2 -2
@@ -1805,15 +1805,15 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)md_error (mddev, rdev); else {struct badblocks *bb = &rdev->badblocks; -u64 *bbp = (u64 *)page_address (rdev->bb_page);u64 *p = bb->page; sb->feature_map |=cpu_to_le32 (MD_FEATURE_BAD_BLOCKS );if (bb->changed) {unsigned seq; +u64 *bbp; retry: + bbp = (u64 *)page_address (rdev->bb_page); seq =read_seqbegin (&bb->lock); -memset (bbp,0xff ,PAGE_SIZE );for (i =0 ; i < bb->count ; i++) {
Signature · 1 line
1.7.9.5
Quoted 26 lines · On Tue, 6 Nov 2012 17:13:00 +0800 majianpeng <[email protected]> wrote:
If
read_seqretryreturned true and bbp was changed, it will write invalid address which can cause some serious problem.This bug was introduced by commit v3.0-rc7-130-g2699b67. So fix is suitable for 3.0.y thru 3.6.y.
Reported-by [email protected]Tested-by [email protected]Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/md/md.c +0
-1@@ -1805,15 +1805,15 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)md_error (mddev, rdev); else {struct badblocks *bb = &rdev->badblocks; -u64 *bbp = (u64 *)page_address (rdev->bb_page);u64 *p = bb->page; sb->feature_map |=cpu_to_le32 (MD_FEATURE_BAD_BLOCKS );if (bb->changed) {
drivers/md/md.c static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev) · line 1819
Quoted 10 lines
drivers/md/md.c +2
-1unsigned seq; +u64 *bbp; retry: + bbp = (u64 *)page_address (rdev->bb_page); seq =read_seqbegin (&bb->lock); -memset (bbp,0xff ,PAGE_SIZE );for (i =0 ; i < bb->count ; i++) {
No.
The contents of the page might change, but it is always the same page, so it always has the same address, so "bbp" is guaranteed to be stable.
NeilBrown
Quoted 37 lines · On Tue, 6 Nov 2012 17:13:00 +0800 majianpeng <[email protected]> wrote:
Quoted 36 lines
If
read_seqretryreturned true and bbp was changed, it will write invalid address which can cause some serious problem.This bug was introduced by commit v3.0-rc7-130-g2699b67. So fix is suitable for 3.0.y thru 3.6.y.
Reported-by [email protected]Tested-by [email protected]Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/md/md.c +2
-2@@ -1805,15 +1805,15 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)md_error (mddev, rdev); else {struct badblocks *bb = &rdev->badblocks; -u64 *bbp = (u64 *)page_address (rdev->bb_page);u64 *p = bb->page; sb->feature_map |=cpu_to_le32 (MD_FEATURE_BAD_BLOCKS );if (bb->changed) {unsigned seq; +u64 *bbp; retry: + bbp = (u64 *)page_address (rdev->bb_page); seq =read_seqbegin (&bb->lock); -memset (bbp,0xff ,PAGE_SIZE );
Quoted 9 lines
drivers/md/md.c static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev) · line 1819
Quoted 1 line
drivers/md/md.c +0
-0for (i =0 ; i < bb->count ; i++) {No.
The contents of the page might change, but it is always the same page, so it always has the same address, so "bbp" is guaranteed to be stable.NeilBrown
Is my understand wrong?
Quoted 7 lines
u64 *bbp = (u64 *)page_address(rdev->bb_page); u64 *p = bb->page; sb->feature_map |= cpu_to_le32(MD_FEATURE_BAD_BLOCKS); if (bb->changed) { unsigned seq;retry:
Quoted 9 lines
seq = read_seqbegin(&bb->lock);
memset(bbp, 0xff, PAGE_SIZE);
for (i = 0 ; i < bb->count ; i++) {
u64 internal_bb = *p++;
u64 store_bb = ((BB_OFFSET(internal_bb) << 10)
| BB_LEN(internal_bb));
*bbp++ = cpu_to_le64(store_bb);I think bbp will be changed. Is ok?
Quoted 4 lines
} bb->changed = 0; if (read_seqretry(&bb->lock, seq)) goto retry;
NeilBrown
AppliedQuoted 56 lines · On Tue, 6 Nov 2012 18:36:33 +0800 majianpeng <[email protected]> wrote:
Quoted 37 lines · On Tue, 6 Nov 2012 17:13:00 +0800 majianpeng <[email protected]> wrote:
Quoted 36 lines
If
read_seqretryreturned true and bbp was changed, it will write invalid address which can cause some serious problem.This bug was introduced by commit v3.0-rc7-130-g2699b67. So fix is suitable for 3.0.y thru 3.6.y.
Reported-by [email protected]Tested-by [email protected]Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/md/md.c +2
-2@@ -1805,15 +1805,15 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)md_error (mddev, rdev); else {struct badblocks *bb = &rdev->badblocks; -u64 *bbp = (u64 *)page_address (rdev->bb_page);u64 *p = bb->page; sb->feature_map |=cpu_to_le32 (MD_FEATURE_BAD_BLOCKS );if (bb->changed) {unsigned seq; +u64 *bbp; retry: + bbp = (u64 *)page_address (rdev->bb_page); seq =read_seqbegin (&bb->lock); -memset (bbp,0xff ,PAGE_SIZE );Quoted 9 lines
drivers/md/md.c static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev) · line 1819
Quoted 1 line
drivers/md/md.c +0
-0for (i =0 ; i < bb->count ; i++) {No.
The contents of the page might change, but it is always the same page, so it always has the same address, so "bbp" is guaranteed to be stable.NeilBrown
Is my understand wrong?
Quoted 8 lines
u64 *bbp = (u64 *)page_address(rdev->bb_page); u64 *p = bb->page; sb->feature_map |= cpu_to_le32(MD_FEATURE_BAD_BLOCKS); if (bb->changed) { unsigned seq;retry:
seq = read_seqbegin(&bb->lock);
Quoted 8 lines
Quoted 7 lines
memset(bbp, 0xff, PAGE_SIZE); for (i = 0 ; i < bb->count ; i++) { u64 internal_bb = *p++; u64 store_bb = ((BB_OFFSET(internal_bb) << 10) | BB_LEN(internal_bb)); *bbp++ = cpu_to_le64(store_bb);I think bbp will be changed. Is ok?
Ahh yes, I see now.
Applied, thanks.
NeilBrown
Quoted 6 lines
Quoted 4 lines
} bb->changed = 0; if (read_seqretry(&bb->lock, seq)) goto retry;
Quoted 58 lines · On Tue, 6 Nov 2012 18:36:33 +0800 majianpeng <[email protected]> wrote:
Quoted 57 lines
Quoted 38 lines · On Tue, 6 Nov 2012 17:13:00 +0800 majianpeng <[email protected]> wrote:
Quoted 36 lines
If
read_seqretryreturned true and bbp was changed, it will write invalid address which can cause some serious problem.This bug was introduced by commit v3.0-rc7-130-g2699b67. So fix is suitable for 3.0.y thru 3.6.y.
Reported-by [email protected]Tested-by [email protected]Signed-off-by Jianpeng Ma <[email protected]>--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/md/md.c +2
-2@@ -1805,15 +1805,15 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)md_error (mddev, rdev); else {struct badblocks *bb = &rdev->badblocks; -u64 *bbp = (u64 *)page_address (rdev->bb_page);u64 *p = bb->page; sb->feature_map |=cpu_to_le32 (MD_FEATURE_BAD_BLOCKS );if (bb->changed) {unsigned seq; +u64 *bbp; retry: + bbp = (u64 *)page_address (rdev->bb_page); seq =read_seqbegin (&bb->lock); -memset (bbp,0xff ,PAGE_SIZE );Quoted 9 lines
drivers/md/md.c static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev) · line 1819
Quoted 1 line
drivers/md/md.c +0
-0for (i =0 ; i < bb->count ; i++) {No.
The contents of the page might change, but it is always the same page, so it always has the same address, so "bbp" is guaranteed to be stable.NeilBrown
Is my understand wrong?
Quoted 8 lines
u64 *bbp = (u64 *)page_address(rdev->bb_page); u64 *p = bb->page; sb->feature_map |= cpu_to_le32(MD_FEATURE_BAD_BLOCKS); if (bb->changed) { unsigned seq;retry:
seq = read_seqbegin(&bb->lock);
Quoted 10 lines
Quoted 8 lines
Quoted 7 lines
memset(bbp, 0xff, PAGE_SIZE); for (i = 0 ; i < bb->count ; i++) { u64 internal_bb = *p++; u64 store_bb = ((BB_OFFSET(internal_bb) << 10) | BB_LEN(internal_bb)); *bbp++ = cpu_to_le64(store_bb);I think bbp will be changed. Is ok?
My patch missed one parameter:
Quoted 1 line
Quoted 1 line
Quoted 1 line
u64 internal_bb = *p++;
So the code should be:
drivers/md/md.c +2 -2
@@ -1817,10 +1817,10 @@ retry:memset (bbp,0xff ,PAGE_SIZE );for (i =0 ; i < bb->count ; i++) { -u64 internal_bb = *p++; +u64 internal_bb = p[i];u64 store_bb = ((BB_OFFSET (internal_bb) <<10 ) |BB_LEN (internal_bb)); - *bbp++ =cpu_to_le64 (store_bb); + bbp[i] =cpu_to_le64 (store_bb); } bb->changed =0 ;if (read_seqretry (&bb->lock, seq))
I think this is your first wanted!