Re: [PATCH] ocfs2: validate la_size before ocfs2_clear_local_alloc()

[d.morgun@ispras.ru]

On 8/11/26 9:57 AM, Joseph Qi wrote:
> I think we can validate localalloc inode during block read in
> ocfs2_validate_inode_block(), which will drop the duplicate code in
> callers.

Hi Joseph,

Thanks for the suggestion. I moved the la_size check there, and it
does catch the current reproducer during mount. Before dropping the
duplicate checks in ocfs2_begin_local_alloc_recovery() and
ocfs2_load_local_alloc(), I'd like to raise two concerns:

1. ocfs2_begin_local_alloc_recovery() reads the inode with
    OCFS2_BH_IGNORE_CACHE, and ocfs2_read_blocks() skips validation
    for dirty buffers. I traced through jbd2 replay: 
ocfs2_replay_journal()
    calls jbd2_journal_flush() right after jbd2_journal_load(), and
    jbd2_journal_recover() itself calls sync_blockdev(), which should
    clear the dirty flag before ocfs2_begin_local_alloc_recovery() runs.
    So this may not be reachable in practice, but I haven't been able
    to confirm it directly. The current reproducer hits the
    validator with a clean buffer during the initial mount, before
    recovery is reached at all. Is it possible to confirm that the
    buffer is guaranteed to be clean by that point?

2. The check is gated on OCFS2_LOCAL_ALLOC_FL, but
    ocfs2_validate_inode_block() doesn't receive the inode's expected
    type, so it can't verify that the on-disk flags actually match
    what the caller expects. A corrupted local alloc inode with the
    flag cleared would skip the check, even though
    ocfs2_clear_local_alloc() still treats its id2 as i_lab. Is there
    an existing way to check this correspondence? Otherwise, this
    might call for a separate per-type validation helper that takes
    the expected inode type, which could also help elsewhere.

My inclination is: v1 (the check directly in
ocfs2_begin_local_alloc_recovery() and ocfs2_load_local_alloc())
has held up in testing so far, with a small additional fix on my
end. Both call sites operate in the LOCAL_ALLOC_SYSTEM_INODE context
and check la_size before using id2 as i_lab, so they do not depend on
OCFS2_LOCAL_ALLOC_FL for deciding whether to validate la_size.
Returning this new error also made a separate, pre-existing bug in
the recovery thread reachable, for which I have a small follow-up
fix.

If the two concerns with ocfs2_validate_inode_block() can be
addressed without too much churn, I agree it is the better place
for this check, since it would cover every caller uniformly.
Otherwise, I'd rather keep it in v1's approach.

Does this seem reasonable, or would you weigh it differently?

Thanks,
Dmitry