Re: [PATCH vfs.all 08/26] erofs: prevent direct access of bd_inode

[Al Viro <viro@zeniv.linux.org.uk>]

On Sat, Apr 06, 2024 at 05:09:12PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Now that all filesystems stash the bdev file, it's ok to get inode
> for the file.

Looking at the only user of erofs_buf->inode (erofs_bread())...  We
use the inode for two things there - block size calculation (to get
from block number to position in bytes) and access to page cache.
We read in full pages anyway.  And frankly, looking at the callers,
we really would be better off if we passed position in bytes instead
of block number.  IOW, it smells like erofs_bread() having wrong type.

Look at the callers.  With 3 exceptions it's
fs/erofs/super.c:135:   ptr = erofs_bread(buf, erofs_blknr(sb, *offset), EROFS_KMAP);
fs/erofs/super.c:151:           ptr = erofs_bread(buf, erofs_blknr(sb, *offset), EROFS_KMAP);
fs/erofs/xattr.c:84:    it.kaddr = erofs_bread(&it.buf, erofs_blknr(sb, it.pos), EROFS_KMAP);
fs/erofs/xattr.c:105:           it.kaddr = erofs_bread(&it.buf, erofs_blknr(sb, it.pos),
fs/erofs/xattr.c:188:           it->kaddr = erofs_bread(&it->buf, erofs_blknr(sb, it->pos),
fs/erofs/xattr.c:294:           it->kaddr = erofs_bread(&it->buf, erofs_blknr(sb, it->pos),
fs/erofs/xattr.c:339:           it->kaddr = erofs_bread(&it->buf, erofs_blknr(it->sb, it->pos),
fs/erofs/xattr.c:378:           it->kaddr = erofs_bread(&it->buf, erofs_blknr(sb, it->pos),
fs/erofs/zdata.c:943:           src = erofs_bread(&buf, erofs_blknr(sb, pos), EROFS_KMAP);

and all of them actually want the return value + erofs_offset(...).  IOW,
we take a linear position (in bytes).  Divide it by block size (from sb).
Pass the factor to erofs_bread(), where we multiply that by block size
(from inode), see which page will that be in, get that page and return a
pointer *into* that page.  Then we again divide the same position
by block size (from sb) and add the remainder to the pointer returned
by erofs_bread().

IOW, it would be much easier to pass the position directly and to hell
with block size logics.  Three exceptions to that pattern:

fs/erofs/data.c:80:     return erofs_bread(buf, blkaddr, type);
fs/erofs/dir.c:66:              de = erofs_bread(&buf, i, EROFS_KMAP);
fs/erofs/namei.c:103:           de = erofs_bread(&buf, mid, EROFS_KMAP);

Those could bloody well multiply the argument by block size;
the first one (erofs_read_metabuf()) is also interesting - its
callers themselves follow the similar pattern.  So it might be
worth passing it a position in bytes as well...

In any case, all 3 have superblock reference, so they can convert
from blocks to bytes conveniently.  Which means that erofs_bread()
doesn't need to mess with block size considerations at all.

IOW, it might make sense to replace erofs_buf->inode with
pointer to address space.  And use file_mapping() instead of
file_inode() in that patch...