On Thu, Nov 14, 2019 at 09:58:12AM +0100, Jan Kara wrote: > 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... Okay, cool. I'll mark it as such. Thanks! -- Kees Cook
