[Ocfs2-tools-devel] thoughts on xattr fsck

[Joel Becker]

Tiger,
	Here's what I'm thinking.  First, you check the xh_count against
the detected_count.  I think you have to bound by both the max_count
calculation we discussed and the detected_count you originally coded.

  1) Calculate the max_count as the largest N where
     ((N * sizeof(entry)) + (N * 4 /* min value */)) <= available space
  2) Use your old detect_count(), the one that just checks
     IS_LAST_ENTRY() and the name hash if it is a bucket.  But don't let
     it go past the max_count you computed in step (1).  This new
     detected_count is a safer value; it doesn't have as much of a
     problem with memmove().
  3) If xh_count is <= detected_count, do nothing
  4) If xh_count > max_count, ask the user if they want to set xh_count
     to detected_count.  If they don't, you just exit out of checking
     this xattr  object.  If they do, you fix xh_count and continue.

	By checking for a last entry, you avoid the memmove() problem -
you're not memmoving() on the maximum count if you could detect
something smaller.
	If xh_count is less then the detected_count, you just trust
xh_count.  Sure, it's *possible* that xh_count was corrupted and should
be larger, but there's no real sane way to figure that out.  In the data
allocation, if l_next_free_rec is corrupted to be smaller than the
number of actual records in the extent list, we can't actually figure
that out; we don't try to guess that extent records past l_next_free_rec
might be valid.  It's tough, but there's not much we can do.  It's
better to get it wrong by losing a few xattrs than to try and claim more
than were there.  It's far more likely that an xh_count <=
detected_count is valid.
	Next, you build some code to map used parts of a buffer (by
"buffer", I mean the xattr portion of an inode/block/bucket).  You'd
initialize this region with the xh.  Something like this pseudocode:

    #define xe_offset(xh, xe)  ((char *)(xe) - (char *)(xh))
    max_offset = xattr area of the inode/block/bucket, including the xh
    used_map = new_used_map(max_offset);
    set_used(used_map, 0, sizeof(struct ocfs2_xattr_header));
    for (i = 0; i < xh_count; i++) {
	xe = &xh->xh_entries[i];

        /* does the entry itself fit? */
        if (!check_fits(used_map, xe_offset(xh, xe), entry_size)) {
            if (!prompt()) {
                rc = -1;
                break;
            } else
                goto wipe_entry;
        }

        /* does the entry make sense? */
        if (xe->xe_name_offset > max_offset) {
            if (!prompt()) {
                rc = -1;
                break;
            } else
                goto wipe_entry;
        }
        if (xe_value_size is wrong for the is_local()) {
            if (!prompt()) {
                rc = -1;
                break;
            } else
                goto wipe_entry;
        }

        /* Ok, it seems to make sense, now we want to check the value
           area against a map where the entry is valid */
        set_used(used_map, xe_offset(xh, xe), entry_size);
        if (!check_fits(used_map, xe->xe_name_offset,
                        xe->xe_name_len + xe->xe_value_size)) {
            /* If the value wouldn't fit, this entry is invalid, so
               clear it from the map */
            clear_used(used_map, xe_offset(xh, xe), entry_size);
            if (!prompt()) {
                rc = -1;
                break;
            } else
                goto wipe_entry;
        }

        /* Ok, this entry is good! */
        continue;

wipe_entry:
        memmove(); memset();
    }

    dealloc_used_map(used_map);
    return rc;

	The basic idea is that the used_map is a linked list of (offset,
length) tuples.  Then check_fits() can make sure a new region, like the
next entry you're checking or the name+value it points to, doesn't
overlap with any previously seen regions.  This handles the case of
out-of-order name_offsets you mentioned earlier.
	What do you think?

Joel

-- 

"To announce that there must be no criticism of them president, or
 that we are to stand by the president, right or wrong, is not only
 unpatriotic and servile, but is morally treasonable to the American
 public."
	- Theodore Roosevelt

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127