Fix off by one error in page_region_mask()

final is calculated to be the last bit to set (ie inclusive) but when we do the mask shifting final really needs to be first bit not to set.

For example if first and final are both bit 0 (ie only first bit to be set) then mask is completely shifted and becomes all zeroes.

Or if first is 0 and final is 63 then the mask is shifted one bit when it shouldn't be shifted at all.

fs/xfs/linux-2.6/xfs_buf.c +5 -3

@@ -129,15 +129,17 @@ page_region_mask(
  	int		first, final;
 
  	first = BTOPR(offset);
-	final = BTOPRT(offset + length - 1);
-	first = min(first, final);
+	final = BTOPRT(offset + length);
+
+	if (first >= final)
+		return 0UL;
 
  	mask = ~0UL;
  	mask <<= BITS_PER_LONG - (final - first);
  	mask >>= BITS_PER_LONG - (final);
 
  	ASSERT(offset + length <= PAGE_CACHE_SIZE);
-	ASSERT((final - first) < BITS_PER_LONG && (final - first) >= 0);
+	ASSERT((final - first) <= BITS_PER_LONG && (final - first) > 0);
 
  	return mask;
  }

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 8 lines · Lachlan McIlroy

final is calculated to be the last bit to set (ie inclusive) but when we do the mask shifting final really needs to be first bit not to set.

For example if first and final are both bit 0 (ie only first bit to be set) then mask is completely shifted and becomes all zeroes.

Or if first is 0 and final is 63 then the mask is shifted one bit when it shouldn't be shifted at all.

Lachlan, what's the end result of this bug? What's the broken behavior?

Thanks,
-Eric

Quoted 21 lines

fs/xfs/linux-2.6/xfs_buf.c +5 -3

@@ -129,15 +129,17 @@ page_region_mask(
  	int		first, final;
 
  	first = BTOPR(offset);
-	final = BTOPRT(offset + length - 1);
-	first = min(first, final);
+	final = BTOPRT(offset + length);
+
+	if (first >= final)
+		return 0UL;
 
  	mask = ~0UL;
  	mask <<= BITS_PER_LONG - (final - first);
  	mask >>= BITS_PER_LONG - (final);
 
  	ASSERT(offset + length <= PAGE_CACHE_SIZE);
-	ASSERT((final - first) < BITS_PER_LONG && (final - first) >= 0);
+	ASSERT((final - first) <= BITS_PER_LONG && (final - first) > 0);
 

Quoted 8 lines

fs/xfs/linux-2.6/xfs_buf.c +0 -0

  	return mask;
  }

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 10 lines · Eric Sandeen

Quoted 8 lines · Lachlan McIlroy

final is calculated to be the last bit to set (ie inclusive) but when we do the mask shifting final really needs to be first bit not to set.

For example if first and final are both bit 0 (ie only first bit to be set) then mask is completely shifted and becomes all zeroes.

Or if first is 0 and final is 63 then the mask is shifted one bit when it shouldn't be shifted at all.

Lachlan, what's the end result of this bug? What's the broken behavior?

There was no observed bug - well nothing I can tie directly to this code. I found this by inspection while investigating the page bitmap stuff. We have a problem with ia64 going to 64K page size with filesystems that use a filesystem sector size of 512 bytes - we don't have the granularity we need in the bitmap.

I suppose it is possible this bug could indicate a page region is not up to date when it actually is and we might re-read something from disk and overwrite the more up to date in-memory version.

Quoted 17 lines · Lachlan McIlroy

Quoted 10 lines · Eric Sandeen

Quoted 8 lines · Lachlan McIlroy

final is calculated to be the last bit to set (ie inclusive) but when we do the mask shifting final really needs to be first bit not to set.

For example if first and final are both bit 0 (ie only first bit to be set) then mask is completely shifted and becomes all zeroes.

Or if first is 0 and final is 63 then the mask is shifted one bit when it shouldn't be shifted at all.

Lachlan, what's the end result of this bug? What's the broken behavior?

There was no observed bug - well nothing I can tie directly to this code. I found this by inspection while investigating the page bitmap stuff. We have a problem with ia64 going to 64K page size with filesystems that use a filesystem sector size of 512 bytes - we don't have the granularity we need in the bitmap.

Quoted 3 lines

I suppose it is possible this bug could indicate a page region is not up to date when it actually is and we might re-read something from disk and overwrite the more up to date in-memory version.

ah, ok. So I've seen this corruption on 64k pages too, on ppc... but I take it this patch doesn't fix it...

final is calculated to be the last bit to set (ie inclusive) but when we do the mask shifting final really needs to be first bit not to set.

For example if first and final are both bit 0 (ie only first bit to be set) then mask is completely shifted and becomes all zeroes.

Or if first is 0 and final is 63 then the mask is shifted one bit when it shouldn't be shifted at all.

fs/xfs/linux-2.6/xfs_buf.c +5 -3

@@ -129,15 +129,17 @@ page_region_mask(
  	int		first, final;
 
  	first = BTOPR(offset);
-	final = BTOPRT(offset + length - 1);
-	first = min(first, final);
+	final = BTOPRT(offset + length);
+
+	if (first >= final)
+		return 0UL;
 
  	mask = ~0UL;
  	mask <<= BITS_PER_LONG - (final - first);
  	mask >>= BITS_PER_LONG - (final);
 
  	ASSERT(offset + length <= PAGE_CACHE_SIZE);
-	ASSERT((final - first) < BITS_PER_LONG && (final - first) >= 0);
+	ASSERT((final - first) <= BITS_PER_LONG && (final - first) > 0);
 
  	return mask;
  }

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 33 lines · Lachlan McIlroy

final is calculated to be the last bit to set (ie inclusive) but when we do the mask shifting final really needs to be first bit not to set.

For example if first and final are both bit 0 (ie only first bit to be set) then mask is completely shifted and becomes all zeroes.

Or if first is 0 and final is 63 then the mask is shifted one bit when it shouldn't be shifted at all.

fs/xfs/linux-2.6/xfs_buf.c +5 -3

@@ -129,15 +129,17 @@ page_region_mask(
  	int		first, final;
 
  	first = BTOPR(offset);
-	final = BTOPRT(offset + length - 1);
-	first = min(first, final);
+	final = BTOPRT(offset + length);
+
+	if (first >= final)
+		return 0UL;
 
  	mask = ~0UL;
  	mask <<= BITS_PER_LONG - (final - first);
  	mask >>= BITS_PER_LONG - (final);
 
  	ASSERT(offset + length <= PAGE_CACHE_SIZE);
-	ASSERT((final - first) < BITS_PER_LONG && (final - first) >= 0);
+	ASSERT((final - first) <= BITS_PER_LONG && (final - first) > 0);
 
  	return mask;
  }

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Gee, I always find these tricky to get right.
I tend to like a userspace version and input various ranges, such as extremes like you did, and verify it is working.

I kind of like first and final to be inclusive of the range to be set (not so keen on making final to be first bit not to set - name doesn't seem so good then).

And if one needs to know the size of the range to use (final - first + 1) and for 0..final => size = final-0+1 = final+1.

And the thing about min(first, final) and (first >= final), is interesting - I would have thought final would be expected to be >= to the first ??

Okay, the other thing that interests me is the use of both BTOPR and BTOPRT for given offsets.
BTOPR is the typical howmany() implementation, where if you go over a multiple-boundaries worth then you need another multiple.
I would expect it to have values starting from 1.
So I was thinking typically of BTOPR for sizes and BTOPRT for 0-based offsets. e.g. given multiple, 512
BTOPR 1..512 => 1, 513..1024 => 2
BTOPRT 0..511 => 0, 512..1023 => 1

So I find the code a bit hard to follow then.

--Tim

Did you guys come to a conclusion in your discussions?

Quoted 2 lines · Christoph Hellwig

Did you guys come to a conclusion in your discussions?

No conclusion, well nothing that resulted in an ack.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 4 lines · Lachlan McIlroy

Quoted 2 lines · Christoph Hellwig

Did you guys come to a conclusion in your discussions?

No conclusion, well nothing that resulted in an ack.

Was anyone okay with this fix?

Quoted 4 lines · Lachlan McIlroy

Quoted 3 lines · Lachlan McIlroy

Quoted 2 lines · Christoph Hellwig

Did you guys come to a conclusion in your discussions?

Quoted 3 lines

Quoted 1 line

No conclusion, well nothing that resulted in an ack.

Was anyone okay with this fix?

I think it's okay to go. It fixes the one off so it's go.

The code is still a very hard to understand mess, but that's not what the patch was supposed to fix..

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Lachlan McIlroy

Quoted 4 lines · Lachlan McIlroy

Quoted 2 lines · Christoph Hellwig

Did you guys come to a conclusion in your discussions?

No conclusion, well nothing that resulted in an ack.

Quoted 6 lines

Quoted 1 line

Was anyone okay with this fix?

I think it's okay to go. It fixes the one off so it's go.

The code is still a very hard to understand mess, but that's not what the patch was supposed to fix..

Any reason this isn't in yet?