Re: Coverity: ext4_iomap_alloc(): Integer handling issues

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Mon, Nov 11, 2019 at 05:35:44PM -0800, coverity-bot wrote:
> This is an experimental automated report about issues detected by Coverity
> from a scan of next-20191108 as part of the linux-next weekly scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
> 
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by recent commits:
> 
> 378f32bab371 ("ext4: introduce direct I/O write using iomap infrastructure")
> 
> Coverity reported the following:
> 
> *** CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /fs/ext4/inode.c: 3388 in ext4_iomap_alloc()
> 3382     	/*
> 3383     	 * We use i_size instead of i_disksize here because delalloc writeback
> 3384     	 * can complete at any point during the I/O and subsequently push the
> 3385     	 * i_disksize out to i_size. This could be beyond where direct I/O is
> 3386     	 * happening and thus expose allocated blocks to direct I/O reads.
> 3387     	 */
> vvv     CID 1487841:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> vvv     Potentially overflowing expression "1 << blkbits" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "loff_t" (64 bits, signed).
> 3388     	else if ((map->m_lblk * (1 << blkbits)) >= i_size_read(inode))
> 3389     		m_flags = EXT4_GET_BLOCKS_CREATE;
> 3390     	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> 3391     		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;

In the event of an overflow in this specific context, I don't think it
would matter too much to be perfectly honest. If 'blkbits' were to
actually ever push out the signed integer to a value that couldn't be
represented by this data type, I would expect the resulting wrapping
behaviour to _only_ affect how filesystem blocks are allocated. In
that case, I/O workloads would behave alot differently, and at that
point I would hope that our filesystem related testing infrastructure
would pick this up before allowing anything to leak out into the
wild...

Unless my trail of thought is wrong? Happy to be corrected here and
educated on this.

/M