On Thu 09-05-24 15:10:07, Justin Stitt wrote: > On Thu, May 9, 2024 at 8:53 AM Jan Kara <jack@xxxxxxx> wrote: > > > @@ -319,8 +320,12 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > > > if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode)) > > > return -ENODEV; > > > > > > - /* Check for wrap through zero too */ > > > - if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0)) > > > + /* Check for wraparound */ > > > + if (check_add_overflow(offset, len, &sum)) > > > + return -EFBIG; > > > + > > > + /* Now, check bounds */ > > > + if (sum > inode->i_sb->s_maxbytes || sum < 0) > > > return -EFBIG; > > > > But why do you check for sum < 0? We know from previous checks offset >= 0 > > && len > 0 so unless we overflow, sum is guaranteed to be > 0. > > Fair enough. I suppose with the overflow check in place we can no > longer have a sum less than zero there. If nothing else, it tells > readers of this code what the domain of (offset+len) is. I don't mind > sending a new version, though. Well, for normal readers offset+len is always a positive number. That's what you expect. If you see a check for offset+len < 0, you start wondering what are you missing... only to find you miss nothing and the check is pointless. So yes, please send a version without the pointless check. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
