[Ocfs2-tools-devel] [PATCH 09/11] ocfs2-tools: add xattr support in fsck.ocfs2

[Tiger Yang]

Joel Becker wrote:
  > 	Boy, I really want this to use the pre_xe->xe_name_offset to
> verify that the entry hasn't walked past the free_start.  There is no
> guarantee that the start of a name value won't be greater than the
> name_hash of the last valid entry.  This would give a false extra entry.
> 	This is why I wish we had a consistent free_start in all types
> of xattr structures, but it's too late for that.  I realize you didn't
> use the xe_name_offset fields here because you haven't validated them
> yet.  So on to that part.

> 	And here, the gap could be corrupt, so we have a similar problem
> to the bucket count code.

> 	I have an idea.  We can't use the offsets to check the count,
> because they might be invalid.  But we can't use the count to check the
> offsets, because it might be invalid.  Tiger, you chose to check the
> count first without the offsets, and then use that count to validate the
> offsets.  How about we do this in two passes.
> 	First, change the above code to merely check a *maximum* count.
> That is, instead of returning 0 when count >= max_count, you just return
> max_count.  Then instead of comparing "if (xh->xh_cout != count)", you
> compare "if (xh->xh_count > count)".  if it happens, you set xh_count to
> count, which will be an upper bound.  That is, as long as xh_count is
> less than or equal to the detected count, we can trust xh_count.  Call
> this check_xattr_count_bounds().
> 	Next, you use this updated xh_count to check the entries.  This
> is safe, because your xh_count is either the same as the detected count
> or less.
> 	After you've checked all the entries, you can use the offsets to
> re-walk the entries and detect if an entry crosses over with the
> name-value section.  Call that check_xattr_count_strict().  You can then
> shrink xh_count if there is a problem.
> 	Finally, you can check the values.
> 

I have tried your suggestion. At first get the maximum count and then 
check entry with it. But I found a problem. In check_entry, when we 
found a entry is invalid, we will remove it, but the entry count is the 
maximum count, so memmove operation will ruin the xattr's name and value.
Because of this, I think we should try our best to get the reasonable 
count before check entry. If we still couldn't get the count, we have to 
remove all xattr entry by set count to zero. check_entry with wrong 
count will cause other problem.
I modified the code for detect count, take your advice use name_offset 
to detect it, so the header gap is not the only method. I attached the 
new code for review.

> 	When you find invalid entries, you leave them in place.  This
> means that name_value_len and free_start get out of sync, because
> name_value_len only covers the good values, but free_start covers all
> values.  I think the kernel handles this ok because it never compares
> free_start and name_value_len, but what happens if someone doesn't
> realize this and changes it?
> 	I suppose we all just watch out for that in future patches.

In fact, when xattr in bucket(and only in bucket it have free_start and 
name_value_len), the free_start and name_value_len may not consistent 
with checking them against each other. I mean free_start + 
name_value_len may not equal with bucket_size, because there might be 
"hole" in value area when we remove a xattr entry.

BTW, I have modified the code according to your other comments, thanks.


thanks,
tiger
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ocfs2-tools-add-xattr-support-in-fsck.ocfs2.patch
Type: text/x-patch
Size: 23385 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-tools-devel/attachments/20090311/b43b6e1e/attachment.bin