On Wed 13-11-19 10:38:43, Kees Cook wrote: > On Wed, Nov 13, 2019 at 10:37:54AM +0100, Jan Kara wrote: > > Well, I don't think we want to clutter various places in the code with > > checks that inode->i_blkbits (which is what blkbits actually is) is what we > > expect. inode->i_blkbits is initialized in fs/inode.c:inode_init_always() > > from sb->s_blocksize_bits and never changed. sb->s_blocksize_bits gets set > > through sb_set_blocksize(). Now it would make sense to assert in > > sb_set_blocksize() that block size is in the range we expect it (currently > > there's just a comment there). But then I suspect that Coverity won't be > > able to carry over the limits as far as into ext4_iomap_alloc() code... > > Kees? > > Yeah, I'm not sure it's capabilities in this regard. It's still a bit of a > black box. :) I just tend to lean toward adding asserts to code-document > value range expectations. Perhaps add the check in sb_set_blocksize() > just because it's a decent thing to test, and if Coverity doesn't notice, > that's okay -- my goal is to improve the kernel which may not always > reduce the static checker noise. :) Now I've noticed that set_blocksize() called from sb_set_blocksize() already has these checks. So there's nothing to add. Just Coverity is not able to carry over those limits that far... Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
