Messages in this thread | | | Date | Wed, 17 May 2006 20:24:13 +0100 (BST) | From | Hugh Dickins <> | Subject | Re: [PATCH] memory mapped files not updating timestamps |
| |
On Wed, 17 May 2006, Peter Staubach wrote: > > Attached are some changes to address the problem that modifications to > the contents of a file, made via an mmap'd region, do not cause the > modification time on the file to be updated. This lack can cause corruption > by allowing backup software to not detect files which should be backed up. > This also represents a potential security hole because it allows a file to be > modified with no corresponding change in the file modification or change > time fields.
It would be grand to fix this (it's, umm, three years since I promised to do so a few days later), but I'm not quite satisfied by your patch as is.
> The changes add support to detect when the modification time needs to be > updated by placing a hook in __set_pages_dirty_buffers and > __set_pages_dirty_nobuffers. One of these two routines will be invoked > when the dirty bit is detected in the pte. The hook sets a new bit in the > address_space mapping struct indicating that the file which is associated > with that part of the address space needs to have its modification and > change time attributes updated.
You're adding a little overhead to every set_page_dirty, when the vast majority (ordinary writes) don't need it: their mctime update is already well taken care of. (Or should we be deleting the code that does that? I think I'd rather not dare.)
Perhaps it's so little overhead that it's not worth worrying about. But writes to a file which happens to be mapped readonly at that time are liable to end up with too late a last mctime on the file, aren't they (if the readonly mapping is unmapped later)?
I think you'd do better to target those places where set_page_dirty is called on a mapped page - and do the file_update_time at that point - or as near to that point as is sensible/permitted given the locking (vma->vm_file gives you the file without needing inode_update_time).
Calling your inode_update_time from unmap_file_vma is probably unwise: at present we may have preemption disabled there, and it's not clear what ->dirty_inode might get up to. Calling it from the vm_file case of remove_vma would be safer.
Peter Zijlstra has patches relating to dirty mmaps in the -mm tree at present: I need to take a look at those, and I'll see if it would make sense to factor in this mctime issue on top of those - you may want to do the same.
In the course of trying that, I'm likely to discover exactly why you made the decisions you did, and arrive at commending your solution.
Hugh - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |