[PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Eric Sandeen]

Regarding the F8 corruption...  I have a pretty narrow testcase
now, and it turns out this was a bit of a perfect storm.

First, F8 shipped 2.6.23 which had the problem with sb_features2
padding out on 64-bit boxes, but this was ok because userspace
and kernelspace both did this, and it was properly mounting &
running as attr2.

However, hch came along in 2.6.24 and did some endian annotation 
for the superblock and in the process:
> A new helper xfs_sb_from_disk handles the other (read)
> direction and doesn't need the slightly hacky table-driven approach
> because we only ever read the full sb from disk.

However, this resulted in kernelspace behaving differently,
and now *missing* the attr2 flag in sb_features2, (actually
sb_bad_features2) so we mounted as if we had attr1.  Which 
is really supposed to be ok, IIRC, except in 
xfs_attr_shortform_bytesfit we return the default
fork offset value from the superblock, even if di_forkoff
is *already* set.  In the error case I had, di_forkoff was set
to 15 (from previously being attr2...) but we returned 14
(the mp default) and I think this is where things started
going wrong; I think this caused us to write an attr on top
of the extent data.

My understanding of this is that if di_forkoff is non-zero,
we should always be using it for space calculations, regardless
of whether we are mounted with attr2 or not...

and with that, how's this look, to be honest I haven't run it
through QA yet...

I'm not certain if xfs_bmap_compute_maxlevels() may lead
to similar problems....

------------------

always use di_forkoff for when checking for attr space

In the case where we mount a filesystem which was previously
using the attr2 format as attr1, returning the default
mp->m_attroffset instead of the per-inode di_forkoff for
inline attribute fit calculations may result in corruption,
if the existing "attr2" formatted attribute is already taking
more space than the default.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---


Index: linux-2.6-git/fs/xfs/xfs_attr_leaf.c
===================================================================
--- linux-2.6-git.orig/fs/xfs/xfs_attr_leaf.c
+++ linux-2.6-git/fs/xfs/xfs_attr_leaf.c
@@ -166,7 +166,7 @@ xfs_attr_shortform_bytesfit(xfs_inode_t 
 
 	if (!(mp->m_flags & XFS_MOUNT_ATTR2)) {
 		if (bytes <= XFS_IFORK_ASIZE(dp))
-			return mp->m_attroffset >> 3;
+			return dp->i_d.di_forkoff;
 		return 0;
 	}

Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Timothy Shimmin]

Hi Eric,

Eric Sandeen wrote:
> Regarding the F8 corruption...  I have a pretty narrow testcase
> now, and it turns out this was a bit of a perfect storm.
> 
> First, F8 shipped 2.6.23 which had the problem with sb_features2
> padding out on 64-bit boxes, but this was ok because userspace
> and kernelspace both did this, and it was properly mounting &
> running as attr2.
> 
> However, hch came along in 2.6.24 and did some endian annotation 
> for the superblock and in the process:
>> A new helper xfs_sb_from_disk handles the other (read)
>> direction and doesn't need the slightly hacky table-driven approach
>> because we only ever read the full sb from disk.
> 
> However, this resulted in kernelspace behaving differently,
> and now *missing* the attr2 flag in sb_features2, (actually
> sb_bad_features2) so we mounted as if we had attr1.  Which 
> is really supposed to be ok, IIRC, 

Yes, I remember Nathan saying that too ;-)

> except in 
> xfs_attr_shortform_bytesfit we return the default
> fork offset value from the superblock, even if di_forkoff
> is *already* set.  In the error case I had, di_forkoff was set
> to 15 (from previously being attr2...) but we returned 14
> (the mp default) and I think this is where things started
> going wrong; I think this caused us to write an attr on top
> of the extent data.
> 
> My understanding of this is that if di_forkoff is non-zero,
> we should always be using it for space calculations, regardless
> of whether we are mounted with attr2 or not...
> 
That was my understanding as well.
I'll have a look at the code soon and see if I can
see any problems with the change and the consistency
of it all.

Thanks a bunch,
Tim.

> and with that, how's this look, to be honest I haven't run it
> through QA yet...
> 
> I'm not certain if xfs_bmap_compute_maxlevels() may lead
> to similar problems....
> 
> ------------------
> 
> always use di_forkoff for when checking for attr space
> 
> In the case where we mount a filesystem which was previously
> using the attr2 format as attr1, returning the default
> mp->m_attroffset instead of the per-inode di_forkoff for
> inline attribute fit calculations may result in corruption,
> if the existing "attr2" formatted attribute is already taking
> more space than the default.
> 
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> ---
> 
> 
> Index: linux-2.6-git/fs/xfs/xfs_attr_leaf.c
> ===================================================================
> --- linux-2.6-git.orig/fs/xfs/xfs_attr_leaf.c
> +++ linux-2.6-git/fs/xfs/xfs_attr_leaf.c
> @@ -166,7 +166,7 @@ xfs_attr_shortform_bytesfit(xfs_inode_t 
>  
>  	if (!(mp->m_flags & XFS_MOUNT_ATTR2)) {
>  		if (bytes <= XFS_IFORK_ASIZE(dp))
> -			return mp->m_attroffset >> 3;
> +			return dp->i_d.di_forkoff;
>  		return 0;
>  	}
> 

Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Eric Sandeen]

Timothy Shimmin wrote:
> Hi Eric,
> 
> Eric Sandeen wrote:
...


>> My understanding of this is that if di_forkoff is non-zero,
>> we should always be using it for space calculations, regardless
>> of whether we are mounted with attr2 or not...
>>
> That was my understanding as well.
> I'll have a look at the code soon and see if I can
> see any problems with the change and the consistency
> of it all.
> 
> Thanks a bunch,
> Tim.

Thanks.  FWIW, if I install F8 with bona-fide attr2 in effect, with
selinux (so attrs on everything) and then update it while mounted as an
attr1 filesystem, with this patch in place, it does not result in
anything bad as far as xfs_repair can see.   (and it's a big update,
probably touching most files...)

-Eric

Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Timothy Shimmin]

Timothy Shimmin wrote:
> Hi Eric,
> 
> Eric Sandeen wrote:
>> Regarding the F8 corruption...  I have a pretty narrow testcase
>> now, and it turns out this was a bit of a perfect storm.
>>
>> First, F8 shipped 2.6.23 which had the problem with sb_features2
>> padding out on 64-bit boxes, but this was ok because userspace
>> and kernelspace both did this, and it was properly mounting &
>> running as attr2.
>>
>> However, hch came along in 2.6.24 and did some endian annotation for 
>> the superblock and in the process:
>>> A new helper xfs_sb_from_disk handles the other (read)
>>> direction and doesn't need the slightly hacky table-driven approach
>>> because we only ever read the full sb from disk.
>>
>> However, this resulted in kernelspace behaving differently,
>> and now *missing* the attr2 flag in sb_features2, (actually
>> sb_bad_features2) so we mounted as if we had attr1.  Which is really 
>> supposed to be ok, IIRC, 
> 
> Yes, I remember Nathan saying that too ;-)
> 
>> except in xfs_attr_shortform_bytesfit we return the default
>> fork offset value from the superblock, even if di_forkoff
>> is *already* set.  In the error case I had, di_forkoff was set
>> to 15 (from previously being attr2...) but we returned 14
>> (the mp default) and I think this is where things started
>> going wrong; I think this caused us to write an attr on top
>> of the extent data.
>>
>> My understanding of this is that if di_forkoff is non-zero,
>> we should always be using it for space calculations, regardless
>> of whether we are mounted with attr2 or not...
>>
> That was my understanding as well.
> I'll have a look at the code soon and see if I can
> see any problems with the change and the consistency
> of it all.
> 
> Thanks a bunch,
> Tim.
> 
>> and with that, how's this look, to be honest I haven't run it
>> through QA yet...
>>
>> I'm not certain if xfs_bmap_compute_maxlevels() may lead
>> to similar problems....
>>
Yes, I think it needs to be changed too.
Looking at the code, it is needed for the log reservation stuff
where we want to know the size of the space for an EA btree root
or a data fork extents root, to aid in working out the maximum
btree level, to work out maximum log space for some transactions.
So underestimating it is dangerous and overestimating is
wasteful of logspace.
It is basing on the same assumption that if we are not mounted
attr2 then it thinks we are using m_attroffset when we could
be using di_forkoff.
However, if di_forkoff is zero then we should use m_attroffset.

The other places which referenced m_attroffset look fine to me too.

>> ------------------
>>
>> always use di_forkoff for when checking for attr space
>>
>> In the case where we mount a filesystem which was previously
>> using the attr2 format as attr1, returning the default
>> mp->m_attroffset instead of the per-inode di_forkoff for
>> inline attribute fit calculations may result in corruption,
>> if the existing "attr2" formatted attribute is already taking
>> more space than the default.
>>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>
>>
>> Index: linux-2.6-git/fs/xfs/xfs_attr_leaf.c
>> ===================================================================
>> --- linux-2.6-git.orig/fs/xfs/xfs_attr_leaf.c
>> +++ linux-2.6-git/fs/xfs/xfs_attr_leaf.c
>> @@ -166,7 +166,7 @@ xfs_attr_shortform_bytesfit(xfs_inode_t  
>>      if (!(mp->m_flags & XFS_MOUNT_ATTR2)) {
>>          if (bytes <= XFS_IFORK_ASIZE(dp))
>> -            return mp->m_attroffset >> 3;
>> +            return dp->i_d.di_forkoff;
>>          return 0;
>>      }
>>
> 
Okay, and XFS_IFORK_ASIZE(dp) looks at di_forkoff and if non-zero
then returns literal-size - di_forkoff.
Right, so it will only fit if di_forkoff is operational.
Cool.

--Tim

Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Timothy Shimmin]

Timothy Shimmin wrote:
> Timothy Shimmin wrote:
>> Hi Eric,
>>
>> Eric Sandeen wrote:
>>> Regarding the F8 corruption...  I have a pretty narrow testcase
>>> now, and it turns out this was a bit of a perfect storm.
>>>
>>> First, F8 shipped 2.6.23 which had the problem with sb_features2
>>> padding out on 64-bit boxes, but this was ok because userspace
>>> and kernelspace both did this, and it was properly mounting &
>>> running as attr2.
>>>
>>> However, hch came along in 2.6.24 and did some endian annotation for 
>>> the superblock and in the process:
>>>> A new helper xfs_sb_from_disk handles the other (read)
>>>> direction and doesn't need the slightly hacky table-driven approach
>>>> because we only ever read the full sb from disk.
>>>
>>> However, this resulted in kernelspace behaving differently,
>>> and now *missing* the attr2 flag in sb_features2, (actually
>>> sb_bad_features2) so we mounted as if we had attr1.  Which is really 
>>> supposed to be ok, IIRC, 
>>
>> Yes, I remember Nathan saying that too ;-)
>>
>>> except in xfs_attr_shortform_bytesfit we return the default
>>> fork offset value from the superblock, even if di_forkoff
>>> is *already* set.  In the error case I had, di_forkoff was set
>>> to 15 (from previously being attr2...) but we returned 14
>>> (the mp default) and I think this is where things started
>>> going wrong; I think this caused us to write an attr on top
>>> of the extent data.
>>>
>>> My understanding of this is that if di_forkoff is non-zero,
>>> we should always be using it for space calculations, regardless
>>> of whether we are mounted with attr2 or not...
>>>
>> That was my understanding as well.
>> I'll have a look at the code soon and see if I can
>> see any problems with the change and the consistency
>> of it all.
>>
>> Thanks a bunch,
>> Tim.
>>
>>> and with that, how's this look, to be honest I haven't run it
>>> through QA yet...
>>>
>>> I'm not certain if xfs_bmap_compute_maxlevels() may lead
>>> to similar problems....
>>>
> Yes, I think it needs to be changed too.
> Looking at the code, it is needed for the log reservation stuff
> where we want to know the size of the space for an EA btree root
> or a data fork extents root, to aid in working out the maximum
> btree level, to work out maximum log space for some transactions.
> So underestimating it is dangerous and overestimating is
> wasteful of logspace.
> It is basing on the same assumption that if we are not mounted
> attr2 then it thinks we are using m_attroffset when we could
> be using di_forkoff.
> However, if di_forkoff is zero then we should use m_attroffset.
> 
Hmmm.
The only problem there is that it is done at mount time assuming
the worst case over all inodes.
So I can't use any di_forkoff but will have to assume like for attr2
the worst case of XFS_BMDR_SPACE_CALC(MIN?BTPTRS).
(Okay I wasn't thinking there ;-)

--tim

Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Timothy Shimmin]

Eric,

So probably need something like this for xfs_bmap_compute_maxlevels()...


--Tim

===========================================================================
Index: fs/xfs/xfs_bmap.c
===========================================================================

--- a/fs/xfs/xfs_bmap.c	2008-03-31 16:32:24.000000000 +1000
+++ b/fs/xfs/xfs_bmap.c	2008-03-31 16:21:11.439806073 +1000
@@ -4154,16 +4154,21 @@ xfs_bmap_compute_maxlevels(
  	 * number of leaf entries, is controlled by the type of di_nextents
  	 * (a signed 32-bit number, xfs_extnum_t), or by di_anextents
  	 * (a signed 16-bit number, xfs_aextnum_t).
+	 *
+	 * Note that we can no longer assume that if we are in ATTR1 that
+	 * the fork offset of all the inodes will be (m_attroffset >> 3)
+	 * because we could have mounted with ATTR2 and then mounted back
+	 * with ATTR1, keeping the di_forkoff's fixed but probably at
+	 * various positions. Therefore, for both ATTR1 and ATTR2
+	 * we have to assume the worst case scenario of a minimum size
+	 * available.
  	 */
  	if (whichfork == XFS_DATA_FORK) {
  		maxleafents = MAXEXTNUM;
-		sz = (mp->m_flags & XFS_MOUNT_ATTR2) ?
-			XFS_BMDR_SPACE_CALC(MINDBTPTRS) : mp->m_attroffset;
+		sz = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
  	} else {
  		maxleafents = MAXAEXTNUM;
-		sz = (mp->m_flags & XFS_MOUNT_ATTR2) ?
-			XFS_BMDR_SPACE_CALC(MINABTPTRS) :
-			mp->m_sb.sb_inodesize - mp->m_attroffset;
+		sz = XFS_BMDR_SPACE_CALC(MINABTPTRS);
  	}
  	maxrootrecs = (int)XFS_BTREE_BLOCK_MAXRECS(sz, xfs_bmdr, 0);
  	minleafrecs = mp->m_bmap_dmnr[0];

Re: [PATCH, RFC] fix attr fit checking for filesystems which have lost their attr2

[Eric Sandeen]

Timothy Shimmin wrote:
> Eric,
> 
> So probably need something like this for xfs_bmap_compute_maxlevels()...

Yep, I think that looks good.  I had forgotten that you could mount with
"noattr2" to change the default behavior... so this isn't only a bug
w.r.t. bad_features2, is it.

Thanks,

-Eric

> 
> --Tim
> 
> ===========================================================================
> Index: fs/xfs/xfs_bmap.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_bmap.c	2008-03-31 16:32:24.000000000 +1000
> +++ b/fs/xfs/xfs_bmap.c	2008-03-31 16:21:11.439806073 +1000
> @@ -4154,16 +4154,21 @@ xfs_bmap_compute_maxlevels(
>   	 * number of leaf entries, is controlled by the type of di_nextents
>   	 * (a signed 32-bit number, xfs_extnum_t), or by di_anextents
>   	 * (a signed 16-bit number, xfs_aextnum_t).
> +	 *
> +	 * Note that we can no longer assume that if we are in ATTR1 that
> +	 * the fork offset of all the inodes will be (m_attroffset >> 3)
> +	 * because we could have mounted with ATTR2 and then mounted back
> +	 * with ATTR1, keeping the di_forkoff's fixed but probably at
> +	 * various positions. Therefore, for both ATTR1 and ATTR2
> +	 * we have to assume the worst case scenario of a minimum size
> +	 * available.
>   	 */
>   	if (whichfork == XFS_DATA_FORK) {
>   		maxleafents = MAXEXTNUM;
> -		sz = (mp->m_flags & XFS_MOUNT_ATTR2) ?
> -			XFS_BMDR_SPACE_CALC(MINDBTPTRS) : mp->m_attroffset;
> +		sz = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
>   	} else {
>   		maxleafents = MAXAEXTNUM;
> -		sz = (mp->m_flags & XFS_MOUNT_ATTR2) ?
> -			XFS_BMDR_SPACE_CALC(MINABTPTRS) :
> -			mp->m_sb.sb_inodesize - mp->m_attroffset;
> +		sz = XFS_BMDR_SPACE_CALC(MINABTPTRS);
>   	}
>   	maxrootrecs = (int)XFS_BTREE_BLOCK_MAXRECS(sz, xfs_bmdr, 0);
>   	minleafrecs = mp->m_bmap_dmnr[0];
>