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 linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Dec 2019 19:11:36 +0530 From: Ritesh Harjani <riteshh@...ux.ibm.com> To: Jan Kara <jack@...e.cz> Cc: tytso@....edu, linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org, mbobrowski@...browski.org, joseph.qi@...ux.alibaba.com Subject: Re: [PATCHv4 3/3] ext4: Move to shared i_rwsem even without dioread_nolock mount opt On 12/5/19 5:35 PM, Jan Kara wrote: > On Thu 05-12-19 12:16:24, Ritesh Harjani wrote: >> We were using shared locking only in case of dioread_nolock mount option in case >> of DIO overwrites. This mount condition is not needed anymore with current code, >> since:- >> >> 1. No race between buffered writes & DIO overwrites. Since buffIO writes takes >> exclusive lock & DIO overwrites will take shared locking. Also DIO path will >> make sure to flush and wait for any dirty page cache data. >> >> 2. No race between buffered reads & DIO overwrites, since there is no block >> allocation that is possible with DIO overwrites. So no stale data exposure >> should happen. Same is the case between DIO reads & DIO overwrites. >> >> 3. Also other paths like truncate is protected, since we wait there for any DIO >> in flight to be over. >> >> 4. In case of buffIO writes followed by DIO reads:- since here also we take >> exclusive lock in ext4_write_begin/end(). There is no risk of exposing any >> stale data in this case. Since after ext4_write_end, iomap_dio_rw() will flush & >> wait for any dirty page cache data to be written. > > The case 4) doesn't seem to be relevant for this patch anymore? Otherwise > the patch looks good to me. You can add: > > Reviewed-by: Jan Kara <jack@...e.cz> > Yup, will remove it. Thanks. > Honza > >> Signed-off-by: Ritesh Harjani <riteshh@...ux.ibm.com> >> --- >> fs/ext4/file.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/fs/ext4/file.c b/fs/ext4/file.c >> index cbafaec9e4fc..682ed956eb02 100644 >> --- a/fs/ext4/file.c >> +++ b/fs/ext4/file.c >> @@ -392,8 +392,8 @@ static const struct iomap_dio_ops ext4_dio_write_ops = { >> * - For extending writes case we don't take the shared lock, since it requires >> * updating inode i_disksize and/or orphan handling with exclusive lock. >> * >> - * - shared locking will only be true mostly with overwrites in dioread_nolock >> - * mode. Otherwise we will switch to exclusive i_rwsem lock. >> + * - shared locking will only be true mostly with overwrites. Otherwise we will >> + * switch to exclusive i_rwsem lock. >> */ >> static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from, >> bool *ilock_shared, bool *extend) >> @@ -415,14 +415,11 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from, >> *extend = true; >> /* >> * Determine whether the IO operation will overwrite allocated >> - * and initialized blocks. If so, check to see whether it is >> - * possible to take the dioread_nolock path. >> - * >> + * and initialized blocks. >> * We need exclusive i_rwsem for changing security info >> * in file_modified(). >> */ >> if (*ilock_shared && (!IS_NOSEC(inode) || *extend || >> - !ext4_should_dioread_nolock(inode) || >> !ext4_overwrite_io(inode, offset, count))) { >> inode_unlock_shared(inode); >> *ilock_shared = false; >> -- >> 2.21.0 >>
Powered by blists - more mailing lists