Re: [Xen-devel] [memory sharing] bug on get_page_and_type

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 12:29 +0000 on 21 Feb (1298291344), MaoXiaoyun wrote:
> Hi Tim:
> 
>          More thoughts on this bug.
> 
>          First some questions
> 
> 
>          1) What PGT_writeable_page means to a page?

It means there is a writeable mapping of it in a pagetable somewhere,
either in a validated PV guest's pagetable or a shadow pagetable.
(Probably, writeable p2m entries ought to take this kind of typecount
too.)

>          2) When a page type will be changed to PGT_writeable_page?
> 
>          3) It looks like PGT_writeable_page is not sharable? Since only 
> PGT_none can, right?

Yes, a page can only be PGT_writeable_page or PGT_share* because it can
only have one type at a time.  PGT_none is irrelevant.

>          4) Could I use get_page_type(page, PGT_writeable_page) before every 
> is_p2m_shared() check.

No; PGT_writeable page means something, so you can't just take it
randomly.

>              Since if get_page_type() success, then the page will has no 
> chance to be shared later
> 
>              and if get_page_type() failed, it page mush has type, it is 
> either  PGT_shared_page or other types,
> 
>              if other types, the page still has no chance to be shared.
> 
>              if PGT_shared_page, that's ok, just do usual is_p2m_shared 
> return routine.
> 
> 
> 
>              question is, is it ok if we only get_page_type, and not to 
> put_page_type()?

No, that's never OK.  Every reference count and typecount must have a
matching put somewhere or we wouldn't be able to re-use memory for new
domains. 

Cheers,

Tim.

> 
> 
> 
> > Date: Fri, 11 Feb 2011 09:32:18 +0000
> > From: Tim.Deegan@xxxxxxxxxx
> > To: tinnycloud@xxxxxxxxxxx
> > CC: xen-devel@xxxxxxxxxxxxxxxxxxx; George.Dunlap@xxxxxxxxxxxxx; 
> > juihaochiang@xxxxxxxxx
> > Subject: Re: [Xen-devel] [memory sharing] bug on get_page_and_type
> >
> > At 07:04 +0000 on 11 Feb (1297407854), MaoXiaoyun wrote:
> > > Thanks Tim.
> > >
> > > After discuss with JuiHao, How about fix in this way?
> > >
> > > 1) Suppose we have a function, make_page_unsharable() to substitude
> > > p2m_is_shared() check, if p2mt is not shared, we increase its type count
> > > to avoid it turn to shared while using it.
> >
> > That's a good idea. I'd rather not have the name be anything to do with
> > "sharable", but we could have a function that does a p2m lookup and a
> > get-page-and-type, all under the p2m lock, and use it instead of the
> > lookup-check-getref idiom elsewhere.< BR>>
> > Then if (as you say) the make-shareable and nominate-page actions were
> > covered by the same lock (or potentially even just called the same
> > function themselves) we would eliminate a lot of races.
> >
> > That will be too big a patch to take before 4.1.0 but I'd consider it
> > immediately after the release.
> >
> > Tim.
> >
> > > 1 int make_page_unsharable(int enable)
> > > 2 {
> > > 3 p2m_type_t p2mt;
> > > 4 unsigned long mfn;
> > > 5
> > > 6 p2m_lock()
> > > 7 mfn = mfn_x(gfn_to_mfn(d, gmfn, &p2mt))
> > > 8
> > > 9 if(p2m_is_shared(p2mt)){
> > > 10 p2m_unlock()
> > > 11 return 1;
> > > 12 }
> > > 13
> > > 14 get_page_type() / ***increase page type count to avoid page type turn 
> > > to shared, since in
> > > mem_sharing_nominate_page->page_make_sharable, only type count zero is
> > > allowed to be shared */
> > > 15 p2m_unlock()
> > > 16
> > > 17 return 0;
> > > 18 }
> > >
> > > 2) If p2mt is not shared, we must decrease it type count after we finish 
> > > using it
> > > 3) To avoid competition, page_make_sharble() and p2m_change_type() in
> > > mem_sharing_nominate_page() should be protected in same p2m_lock.
> > >
> > > comments?
> > >
> > >
> > > > Date: Wed, 9 Feb 2011 09:57:20 +0000
> > > > From: Tim.Deegan@xxxxxxxxxx
> > > > To: tinnycloud@xxxxxxxxxxx
> > > > CC: xen-devel@xxxxxxxxxxxxxxxxxxx; George.Dunlap@xxxxxxxxxxxxx; 
> > > > juihaochiang@xxxxxxxxx
> > > > Subject: Re: [Xen-devel] [memory sharing] bug on get_page_and_type
> > > >
> > > > At 02:46 +0000 on 09 Feb (1297219562), MaoXiaoyun wrote:
> > > > > I've been looking into the TOCTOU issue quite a while, but
> > > > >
> > > > > 1) Th ere are quite a lot judgements like "p2m_is_shared(p2mt)" or
> > > > > "p2mt == p2m_ram_shared", which, for me, is hard to tell whom
> > > > > are need to be protect by p2m_lock and whom are not So is
> > > > > there a rule to distinguish these?
> > > >
> > > > Not particularly. I suspect that most of them will need to be
> > > > changed, but as I said I hope we can find something nicer than
> > > > scattering p2m_lock() around non-p2m code.
> > > >
> > > > > 2) Could we improve p2m_lock to sparse lock, which maybe better, 
> > > > > right?
> > > >
> > > > Maybe, but not necessarily. Let's get it working properly first and
> > > > then we can measure lock contention and see whether fancy locks are
> > > > worthwhile.
> > > >
> > > > Tim.
> > > >
> > > > >
> > > > > > Date: Wed, 2 Feb 2011 16:18:37 +0000
> > > > > > From: Tim.Deegan@xxxxxxxxxx
> > > > > > To: tinnycloud@xxxxxxxxxxx
> > > > > > CC: xen-devel@xxxxxxxxxxxxxxxxxxx; George.Dunlap@xxxxxxxxxxxxx; 
> > > > > > juihaochiang@xxxxxxxxx
> > > > > > Subject: Re: [Xen-devel] [memory sharing] bug on get_page_and_type
> > > > > >
> > > > > > At 15:43 +0000 on 02 Feb (1296661396), MaoXiaoyun wrote:
> > > > > > > Hi Tim:
> > > > > > >
> > > > > > > Thanks for both your advice and quick reply. I will follow.
> > > > > > >
> > > > > > > So at last we should replace shr_lock with p2m_lock.
> > > > > > > But more complicate, it seems both the
> > > > > > > *check action* code and *nominate page* code need to be locked 
> > > > > > > ,right?
> > > > > > > If so, quite a lot of *check action* codes nee d to be locked.
> > > > > >
> > > > > > Yes, I think you're right about that. Unfortunately there are some 
> > > > > > very
> > > > > > long TOCTOU windows in those kind of p2m lookups, which will get 
> > > > > > more
> > > > > > important as the p2m gets more dynamic. I don't want to have the
> > > > > > callers of p2m code touching the p2m lock directly so we may need a 
> > > > > > new
> > > > > > p2m interface to address it.
> > > > > >
> > > > > > Tim.
> > > > > >
> > > >
> > > > --
> > > > Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> > > > Principal Software Engineer, Xen Platform Team
> > > > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
> >
> > --
> > Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> > Principal Software Engineer, Xen Platform Team
> > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel


-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel