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: xen devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: RE: [Xen-devel] [memory sharing] bug on get_page_and_type
From: MaoXiaoyun <tinnycloud@xxxxxxxxxxx>
Date: Mon, 21 Feb 2011 20:29:04 +0800
Cc: george.dunlap@xxxxxxxxxxxxx, harshan.dll@xxxxxxxxx, tim.deegan@xxxxxxxxxx, juihaochiang@xxxxxxxxx
Delivery-date: Mon, 21 Feb 2011 04:30:16 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
Importance: Normal
In-reply-to: <20110211093218.GE18135@xxxxxxxxxxxxxxxxxxxxxxx>
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-ds63E3E55F8D444780B613ADAE20@xxxxxxx>, <AANLkTika2afgs8hhdMD024PHGzDFsoWcOK7nJ7ePzG=z@xxxxxxxxxxxxxx>, <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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Hi Tim:
 
         More thoughts on this bug.
        
         First some questions
 

         1) What PGT_writeable_page means to a page?

         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?

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

              

             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()?

       


 
> 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