On Fri, Jan 17, 2020 at 10:27 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > Thanks. > чт, 16 янв. 2020 г. в 01:05, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>: > > > > The bug is basically that if we extend a file by fallocate mode==0 > > and immediately afterwards mmap() the file we will only mmap() as much > > as end-of-file was > > prior to the truncate and then if we try to touch any > > address in this extended region userspace dies with bus error. > > > > The patch added "extend a file with fallocate mode==0 for NON-Sparse > > files" and caused xfstest to fail. > > The fix is to force us to revalidate the file attributes (the size is > > the important one) when we extend the file so > > mmap() will work properly. > > I have fixed this in the patch and will resend tomorrow after some more testing. > > > > Looking for other SMB2_set_eof() callsites I see we already had the > > same bug for the case of extending a SPARSE > > I agree that regardless of file being sparse or not, we should somehow > update a size in the VFS after calling SMB2_set_eof(). > > > file using fallocate mode==0. I fixed that too and will audit all > > other plases where we use SMB2_set_eof() > > to see if they are safe or not before resending. > > One of those places where SMB2_set_eof() is called is > cifs_set_file_size() which does call the following after getting a > successful response from the server: > > 2250 >-------if (rc == 0) { > 2251 >------->-------cifsInode->server_eof = attrs->ia_size; > 2252 >------->-------cifs_setsize(inode, attrs->ia_size); > 2253 >------->-------cifs_truncate_page(inode->i_mapping, inode->i_size); > 2254 >-------} > > This is called by cifs_setattr_[no]unix() which does the following afterwards: > > 2569 >-------if ((attrs->ia_valid & ATTR_SIZE) && > 2570 >------- attrs->ia_size != i_size_read(inode)) > 2571 >------->-------truncate_setsize(inode, attrs->ia_size); > > truncate_setsize() does similar things as cifs_setsize() besides > setting cinode->time to 0. This code path probably needs to be > refactored. But putting this aside, for the current fallocate fix I > think we should use the same existing mechanism to update a file size > in the VFS without revalidating the inode. I can do that change. Note however that since we are still setting cinode->time to 0 we are still going to trigger a revalidation at some stage. > > -- > Best regards, > Pavel Shilovsky