WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: MaoXiaoyun <tinnycloud@xxxxxxxxxxx>
Subject: Re: [Xen-devel] [memory sharing] bug on get_page_and_type
From: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Date: Mon, 21 Feb 2011 14:08:55 +0000
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, xen devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, "harshan.dll@xxxxxxxxx" <harshan.dll@xxxxxxxxx>, "juihaochiang@xxxxxxxxx" <juihaochiang@xxxxxxxxx>
Delivery-date: Mon, 21 Feb 2011 06:10:18 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <BLU157-w48B4D445EBDB7B01810999DAD90@xxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <BLU157-w29AAD53D1BF4AF2892205EDAE20@xxxxxxx> <AANLkTim4O6QXdAJedB1KhECpDhrq2w8FTDXn7irQtk2Y@xxxxxxxxxxxxxx> <20110201142816.GE20638@xxxxxxxxxxxxxxxxxxxxxxx> <BLU157-w243AEA6AA94C1835B206F4DAE40@xxxxxxx> <20110202161837.GQ8286@xxxxxxxxxxxxxxxxxxxxxxx> <BLU157-w23DF5D68A8E1E7781B72BADAED0@xxxxxxx> <20110209095720.GC18135@xxxxxxxxxxxxxxxxxxxxxxx> <BLU157-w29BD737351D22849AB4F5DAEF0@xxxxxxx> <20110211093218.GE18135@xxxxxxxxxxxxxxxxxxxxxxx> <BLU157-w48B4D445EBDB7B01810999DAD90@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.20 (2009-06-14)
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