lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 31 Mar 2022 11:03:59 -0700 From: Tadeusz Struk <tadeusz.struk@...aro.org> To: Theodore Ts'o <tytso@....edu> Cc: Andreas Dilger <adilger.kernel@...ger.ca>, Ritesh Harjani <riteshh@...ux.ibm.com>, linux-ext4@...r.kernel.org, stable@...r.kernel.org, linux-kernel@...r.kernel.org, syzbot+7a806094edd5d07ba029@...kaller.appspotmail.com Subject: Re: [PATCH v2] ext4: check if offset+length is valid in fallocate On 3/31/22 07:43, Theodore Ts'o wrote: > On Tue, Mar 15, 2022 at 02:54:39PM -0700, Tadeusz Struk wrote: >> @@ -3967,6 +3968,16 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length) >> offset; >> } >> >> + /* >> + * For punch hole the length + offset needs to be at least within >> + * one block before last >> + */ >> + max_length = sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize; >> + if (offset + length >= max_length) { >> + ret = -ENOSPC; >> + goto out_mutex; >> + } > > I wonder if we would be better off just simply capping length to > max_length? If length is set to some large value, such as LONG_MAX, > it's pretty clear what the intention should be, which is to simply do > the equivalent of truncating the file at offset. Perhaps we should > just do that? Don't think that would be the correct behavior. ftrucnate (or truncate) modify the file size, but fallocate with FALLOC_FL_PUNCH_HOLE should not. man 2 fallocate says: "... The FALLOC_FL_PUNCH_HOLE flag must be ORed with FALLOC_FL_KEEP_SIZE in mode; in other words, even when punching off the end of the file, the file size (as reported by stat(2)) does not change. " that is enforced by vfs: https://elixir.bootlin.com/linux/v5.17.1/source/fs/open.c#L245 > > That being said, we should be consistent with what other file systems > do when they are asked to punch a hole starting at offset and > extending out to LONG_MAX. For all the supported file systems, apart from ext4, only btrfs, gfs2, and xfs support fallocate and FALLOC_FL_PUNCH_HOLE mode. Looking at what they do is they round the length of the space to be freed i.e. offset + length to valid value and then perform the operation using the valid values. https://elixir.bootlin.com/linux/v5.17.1/source/fs/gfs2/bmap.c#L2424 https://elixir.bootlin.com/linux/v5.17.1/source/fs/btrfs/file.c#L2506 For ext4 this would mean that one could only deallocate space up to the one before last block. I will change this to do the same in the next version if that's ok with you. > > Also, if we are going to return an error, I don't think ENOSPC is the > correct error to be returning. I took it from man 2 fallocate, my first suspicion was that it crashed because the disk on my VM wasn't big enough. It was a bad choice. -- Thanks, Tadeusz
Powered by blists - more mailing lists