On Sun, Sep 15, 2024 at 08:05:46AM +0100, Al Viro wrote:
> Interesting... How does the mainline manage to avoid the
> call of exfat_kill_sb(), which should call_rcu() delayed_free(), which
> calls exfat_free_upcase_table()?
>
> Could you verify that your reproducer does *NOT* hit that
> callchain? AFAICS, the only caller of exfat_load_upcase_table()
> is exfat_create_upcase_table(), called by __exfat_fill_super(),
> called by exfat_fill_super(), passed as callback to get_tree_bdev().
> And if that's the case, ->kill_sb() should be called on failure and
> with non-NULL ->s_fs_info...
>
> Something odd is going on there.
Yecchh... OK, I see what's happening, and the patch is probably
correct, but IMO it's way too subtle. Unless I'm misreading what's
going on there, you have the following:
exfat_load_upcase_table() have 3 failure exits.
One of them is with -ENOMEM; no table allocated and we proceed to
exfat_load_default_upcase_table().
Another is with -EIO. In that case the table is left allocated, the
caller of exfat_load_upcase_table() returns immediately and the normal
logics in ->kill_sb() takes it out.
Finally, there's one with -EINVAL. There the caller proceeds to
exfat_load_default_upcase_table(), which is where the mainline leaks.
That's the case your patch adjusts.
Note that resulting rules for exfat_load_upcase_table()
* should leave for ->kill_sb() to free if failing with -EIO.
* should make sure it's freed on all other failure exits.
At the very least that needs to be documented. However, since the
problem happens when the caller proceeds to exfat_load_default_upcase_table(),
the things would be simpler if you had taken the "need to free what we'd
allocated" logics into the place where that logics is visible. I.e.
ret = exfat_load_upcase_table(sb, sector, num_sectors,
le32_to_cpu(ep->dentry.upcase.checksum));
brelse(bh);
if (ret && ret != -EIO) {
/* clean after exfat_load_upcase_table() */
exfat_free_upcase_table(sbi);
goto load_default;
}
IMO it would be less brittle that way. And commit message needs
the explanation of the leak mechanism - a link to reporter is
nice, but it doesn't explain what's going on.
