[Ocfs2-devel] [PATCH 2/2] Ocfs2/move_extents: Validate moving goal after the adjustment.

[Ocfs2-devel] [PATCH 2/2] Ocfs2/move_extents: Validate moving goal after the adjustment.

[Mariusz Kozlowski]

Hi,

  I think this one (ea5e1675 upstream) is wrong. Validation was moved
before 'bg' was assinged any sane value. Also 'bg' is defined with NULL
now so it hides real problem that 'bg' is used uninitialized. So currently
as 'bg' is NULL from the begining it will blow up with null pointer
dereference somewhere around test in line 489:

489:        if (range->me_goal == le64_to_cpu(bg->bg_blkno))
490:	                range->me_goal += c_to_b;

Sorry for reply with no context but I'm not subscribed to ocfs2-devel.
-- 
Mariusz Kozlowski

[Ocfs2-devel] [PATCH 2/2] Ocfs2/move_extents: Validate moving goal after the adjustment.

[Tristan Ye]

On 06/12/2011 03:57 AM, Mariusz Kozlowski wrote:
> Hi,
> 
>   I think this one (ea5e1675 upstream) is wrong. Validation was moved
> before 'bg' was assinged any sane value. Also 'bg' is defined with NULL
> now so it hides real problem that 'bg' is used uninitialized. So currently
> as 'bg' is NULL from the begining it will blow up with null pointer
> dereference somewhere around test in line 489:


You're definitely correct, I'm blaming myself for not making things in
order after moving the validation logic backwards a bit, thanks so much
for pointing this out.

Tristan.

> 
> 489:        if (range->me_goal == le64_to_cpu(bg->bg_blkno))
> 490:	                range->me_goal += c_to_b;
> 
> Sorry for reply with no context but I'm not subscribed to ocfs2-devel.

[Ocfs2-devel] [PATCH 1/2] Ocfs2/move_extents: Avoid doing division in extent moving.

[Tristan Ye]

It's not wise enough to do a 64bits division anywhere in kernside, replace it
with a decent helper or proper shifts.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/move_extents.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
index 4c54884..d184e0b 100644
--- a/fs/ocfs2/move_extents.c
+++ b/fs/ocfs2/move_extents.c
@@ -368,7 +368,7 @@ static int ocfs2_find_victim_alloc_group(struct inode *inode,
 					 int *vict_bit,
 					 struct buffer_head **ret_bh)
 {
-	int ret, i, blocks_per_unit = 1;
+	int ret, i, bits_per_unit = 0;
 	u64 blkno;
 	char namebuf[40];
 
@@ -398,14 +398,14 @@ static int ocfs2_find_victim_alloc_group(struct inode *inode,
 	rec = &(cl->cl_recs[0]);
 
 	if (type == GLOBAL_BITMAP_SYSTEM_INODE)
-		blocks_per_unit <<= (osb->s_clustersize_bits -
-						inode->i_sb->s_blocksize_bits);
+		bits_per_unit = osb->s_clustersize_bits -
+					inode->i_sb->s_blocksize_bits;
 	/*
 	 * 'vict_blkno' was out of the valid range.
 	 */
 	if ((vict_blkno < le64_to_cpu(rec->c_blkno)) ||
-	    (vict_blkno >= (le32_to_cpu(ac_dinode->id1.bitmap1.i_total) *
-				blocks_per_unit))) {
+	    (vict_blkno >= (le32_to_cpu(ac_dinode->id1.bitmap1.i_total) <<
+				bits_per_unit))) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -441,8 +441,8 @@ static int ocfs2_find_victim_alloc_group(struct inode *inode,
 						le16_to_cpu(bg->bg_bits))) {
 
 				*ret_bh = gd_bh;
-				*vict_bit = (vict_blkno - blkno) /
-							blocks_per_unit;
+				*vict_bit = (vict_blkno - blkno) >>
+							bits_per_unit;
 				mlog(0, "find the victim group: #%llu, "
 				     "total_bits: %u, vict_bit: %u\n",
 				     blkno, le16_to_cpu(bg->bg_bits),
@@ -493,9 +493,8 @@ static int ocfs2_validate_and_adjust_move_goal(struct inode *inode,
 	/*
 	 * make goal become cluster aligned.
 	 */
-	if (range->me_goal % c_to_b)
-		range->me_goal = range->me_goal / c_to_b * c_to_b;
-
+	range->me_goal = ocfs2_block_to_cluster_start(inode->i_sb,
+						      range->me_goal);
 	/*
 	 * moving goal is not allowd to start with a group desc blok(#0 blk)
 	 * let's compromise to the latter cluster.
-- 
1.5.5

[Ocfs2-devel] [PATCH 2/2] Ocfs2/move_extents: Validate moving goal after the adjustment.

[Tristan Ye]

though the goal_to_be_moved will be validated again in following moving, it's
still a good idea to validate it after adjustment at the very beginning, instead
of validating it before adjustment.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/move_extents.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
index d184e0b..cd94270 100644
--- a/fs/ocfs2/move_extents.c
+++ b/fs/ocfs2/move_extents.c
@@ -472,12 +472,24 @@ static int ocfs2_validate_and_adjust_move_goal(struct inode *inode,
 	int ret, goal_bit = 0;
 
 	struct buffer_head *gd_bh = NULL;
-	struct ocfs2_group_desc *bg;
+	struct ocfs2_group_desc *bg = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	int c_to_b = 1 << (osb->s_clustersize_bits -
 					inode->i_sb->s_blocksize_bits);
 
 	/*
+	 * make goal become cluster aligned.
+	 */
+	range->me_goal = ocfs2_block_to_cluster_start(inode->i_sb,
+						      range->me_goal);
+	/*
+	 * moving goal is not allowd to start with a group desc blok(#0 blk)
+	 * let's compromise to the latter cluster.
+	 */
+	if (range->me_goal == le64_to_cpu(bg->bg_blkno))
+		range->me_goal += c_to_b;
+
+	/*
 	 * validate goal sits within global_bitmap, and return the victim
 	 * group desc
 	 */
@@ -491,18 +503,6 @@ static int ocfs2_validate_and_adjust_move_goal(struct inode *inode,
 	bg = (struct ocfs2_group_desc *)gd_bh->b_data;
 
 	/*
-	 * make goal become cluster aligned.
-	 */
-	range->me_goal = ocfs2_block_to_cluster_start(inode->i_sb,
-						      range->me_goal);
-	/*
-	 * moving goal is not allowd to start with a group desc blok(#0 blk)
-	 * let's compromise to the latter cluster.
-	 */
-	if (range->me_goal == le64_to_cpu(bg->bg_blkno))
-		range->me_goal += c_to_b;
-
-	/*
 	 * movement is not gonna cross two groups.
 	 */
 	if ((le16_to_cpu(bg->bg_bits) - goal_bit) * osb->s_clustersize <
-- 
1.5.5