Re: [FYI] tux3: Core changes

[Jan Kara <jack@suse.cz>]

On Mon 22-06-15 00:36:00, OGAWA Hirofumi wrote:
> Jan Kara <jack@suse.cz> writes:
> > So there are a few things to have in mind:
> > 1) There is nothing like a "writeable" page. Page is always writeable (at
> > least on x86 architecture). When a page is mapped into some virtual address
> > space (or more of them), this *mapping* can be either writeable or read-only.
> > mkwrite changes the mapping from read-only to writeable but kernel /
> > hardware is free to write to the page regardless of the mapping.
> >
> > 2) When kernel / hardware writes to the page, it first modifies the page
> > and then marks it dirty.
> >  
> > So what can happen in this scenario is:
> >
> > 1) You hand kernel a part of a page as a buffer. page_mkwrite() happens,
> >    page is dirtied, kernel notes a PFN of the page somewhere internally.
> >
> > 2) Writeback comes and starts writeback for the page.
> >
> > 3) Kernel ships the PFN to the hardware.
> >
> > 4) Userspace comes and wants to write to the page (different part than the
> >    HW is instructed to use). page_mkwrite is called, page is forked.
> >    Userspace writes to the forked page.
> >
> > 5) HW stores its data in the original page.
> >
> > Userspace never sees data from the HW! Data corrupted where without page
> > forking everything would work just fine.
> 
> 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.

> 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.
 
> For page forking, we may also be able to prevent similar situation by
> locking, flags, and revalidate. But those details might be different
> with current code, because page states are different.

Sorry, I don't understand what do you mean in this paragraph. Can you
explain it a bit more?
 
> > Another possible scenario:
> >
> > 1) Userspace app tells kernel to setup a HW buffer in a page.
> >
> > 2) Userspace app fills page with data -> page_mkwrite is called, page is
> >    dirtied.
> >
> > 3) Userspace app tells kernel to ship buffer to video HW.
> >
> > 4) Writeback comes and starts writeback for the page
> >
> > 5) Video HW is done with the page. Userspace app fills new set of data into
> >    the page -> page_mkwrite is called, page is forked.
> >
> > 6) Userspace app tells kernel to ship buffer to video HW. But HW gets the
> >    old data from the original page.
> >
> > Again a data corruption issue where previously things were working fine.
> 
> This logic seems to be same as above. Replace the page fork at (5).
> With no revalidate of page, (6) will use the old page.

  Yes, the same arguments as above apply...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR