On Sun 05-07-15 21:54:45, OGAWA Hirofumi wrote: > Jan Kara <jack@xxxxxxx> writes: > > >> I'm not sure I'm understanding your pseudocode logic correctly though. > >> This logic doesn't seems to be a page forking specific issue. And > >> this pseudocode logic seems to be missing the locking and revalidate of > >> page. > >> > >> If you can show more details, it would be helpful to see more, and > >> discuss the issue of page forking, or we can think about how to handle > >> the corner cases. > >> > >> Well, before that, why need more details? > >> > >> For example, replace the page fork at (4) with "truncate", "punch > >> hole", or "invalidate page". > >> > >> Those operations remove the old page from radix tree, so the > >> userspace's write creates the new page, and HW still refererences the > >> old page. (I.e. situation should be same with page forking, in my > >> understand of this pseudocode logic.) > > > > Yes, if userspace truncates the file, the situation we end up with is > > basically the same. However for truncate to happen some malicious process > > has to come and truncate the file - a failure scenario that is acceptable > > for most use cases since it doesn't happen unless someone is actively > > trying to screw you. With page forking it is enough for flusher thread > > to start writeback for that page to trigger the problem - event that is > > basically bound to happen without any other userspace application > > interfering. > > Acceptable conclusion is where came from? That pseudocode logic doesn't > say about usage at all. And even if assume it is acceptable, as far as I > can see, for example /proc/sys/vm/drop_caches is enough to trigger, or a > page on non-exists block (sparse file. i.e. missing disk space check in > your logic). And if really no any lock/check, there would be another > races. So drop_caches won't cause any issues because it avoids mmaped pages. Also page reclaim or page migration don't cause any issues because they avoid pages with increased refcount (and increased refcount would stop drop_caches from reclaiming the page as well if it was not for the mmaped check before). Generally, elevated page refcount currently guarantees page isn't migrated, reclaimed, or otherwise detached from the mapping (except for truncate where the combination of mapping-index becomes invalid) and your page forking would change that assumption - which IMHO has a big potential for some breakage somewhere. And frankly I fail to see why you and Daniel care so much about this corner case because from performance POV it's IMHO a non-issue and you bother with page forking because of performance, don't you? > >> IOW, this pseudocode logic seems to be broken without page forking if > >> no lock and revalidate. Usually, we prevent unpleasant I/O by > >> lock_page or PG_writeback, and an obsolated page is revalidated under > >> lock_page. > > > > Well, good luck with converting all the get_user_pages() users in kernel to > > use lock_page() or PG_writeback checks to avoid issues with page forking. I > > don't think that's really feasible. > > What does all get_user_pages() conversion mean? Well, maybe right more > or less, I also think there is the issue in/around get_user_pages() that > we have to tackle. > > > IMO, if there is a code that pseudocode logic actually, it is the > breakage. And "it is acceptable and limitation, and give up to fix", I > don't think it is the right way to go. If there is really code broken > like your logic, I think we should fix. > > Could you point which code is using your logic? Since that seems to be > so racy, I can't believe yet there are that racy codes actually. So you can have a look for example at drivers/media/v4l2-core/videobuf2-dma-contig.c which implements setting up of a video device buffer at virtual address specified by user. Now I don't know whether there really is any userspace video program that sets up the video buffer in mmaped file. I would agree with you that it would be a strange thing to do but I've seen enough strange userspace code that I would not be too surprised. Another example of similar kind is at drivers/infiniband/core/umem.c where we again set up buffer for infiniband cards at users specified virtual address. And there are more drivers in kernel like that. Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html