From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH v15 01/11] multicall: add no preemption ability between two calls Date: Wed, 10 Sep 2014 09:32:32 +0800 Message-ID: <20140910013232.GG15872@pengc-linux> References: <1409906249-6057-1-git-send-email-chao.p.peng@linux.intel.com> <1409906249-6057-2-git-send-email-chao.p.peng@linux.intel.com> <5409947C.3020402@citrix.com> <20140909064359.GF15872@pengc-linux> <540EF4EC0200007800032903@mail.emea.novell.com> <540EDB9B.6080908@citrix.com> <540F05ED020000780003299E@mail.emea.novell.com> <540EF624.4020200@citrix.com> <540F19AF0200007800032AD1@mail.emea.novell.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <540F19AF0200007800032AD1@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, George.Dunlap@eu.citrix.com, Andrew Cooper , Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Tue, Sep 09, 2014 at 02:15:59PM +0100, Jan Beulich wrote: > >>> On 09.09.14 at 14:44, wrote: > > On 09/09/14 12:51, Jan Beulich wrote: > >>>>> On 09.09.14 at 12:51, wrote: > >>> On 09/09/14 11:39, Jan Beulich wrote: > >>>>>>> On 09.09.14 at 08:43, wrote: > >>>>> On Fri, Sep 05, 2014 at 11:46:20AM +0100, Andrew Cooper wrote: > >>>>>> On 05/09/14 09:37, Chao Peng wrote: > >>>>>>> Add a flag to indicate if the execution can be preempted between two > >>>>>>> calls. If not specified, stay preemptable. > >>>>>>> > >>>>>>> Signed-off-by: Chao Peng > >>>>>>> --- > >>>>>>> xen/common/multicall.c | 5 ++++- > >>>>>>> xen/include/public/xen.h | 4 ++++ > >>>>>>> 2 files changed, 8 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/xen/common/multicall.c b/xen/common/multicall.c > >>>>>>> index fa9d910..83b96eb 100644 > >>>>>>> --- a/xen/common/multicall.c > >>>>>>> +++ b/xen/common/multicall.c > >>>>>>> @@ -40,6 +40,7 @@ do_multicall( > >>>>>>> struct mc_state *mcs = ¤t->mc_state; > >>>>>>> uint32_t i; > >>>>>>> int rc = 0; > >>>>>>> + bool_t preemptable = 0; > >>>>>>> > >>>>>>> if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) ) > >>>>>>> { > >>>>>>> @@ -52,7 +53,7 @@ do_multicall( > >>>>>>> > >>>>>>> for ( i = 0; !rc && i < nr_calls; i++ ) > >>>>>>> { > >>>>>>> - if ( i && hypercall_preempt_check() ) > >>>>>>> + if ( preemptable && hypercall_preempt_check() ) > >>>>>>> goto preempted; > >>>>>>> > >>>>>>> if ( unlikely(__copy_from_guest(&mcs->call, call_list, 1)) ) > >>>>>>> @@ -61,6 +62,8 @@ do_multicall( > >>>>>>> break; > >>>>>>> } > >>>>>>> > >>>>>>> + preemptable = mcs->call.flags & MC_NO_PREEMPT; > >>>>>>> + > >>>>>> Please consider what would happen if a malicious guest set NO_PREEMPT on > >>>>>> every multicall entry. > >>>>> OK, I see. My direct purpose here is to support batch operations for > >>>>> XENPF_resource_op added in next patch. Recall what Jan suggested in v14 > >>>>> comments, we have 3 possible ways to support XENPF_resource_op batch: > >>>>> 1) Add a field in the xenpf_resource_op to indicate the iteration; > >>>>> 2) Fiddle multicall mechanism, just like this patch; > >>>>> 3) Add a brand new hypercall. > >>>>> > >>>>> So perhaps I will give up option 2) before I can see any improvement > >>>>> here. While option 3) is aggressive, so I'd go option 1) through I also > >>>>> don't quite like it (Totally because the iteration is transparent for user). > >>>> The I suppose you didn't really understand Andrew's comment: I > >>>> don't think he was suggesting to drop the approach, but instead > >>>> to implement it properly (read: securely). > >>> That is certainly one part of it. > >>> > >>> However, there is the other open question (dropped from this context) of > >>> how to deal with a multicall which has NO_PREEMT set, which itself > >>> preempts, and I don't have a good answer for this. > >> The pretty natural answer to this is - the specific handler knows > >> best what to do. > > > > Given our past history at retrofitting preempting into existing > > hypercalls, the multicaller has no idea whether the ops they have > > selected will preempt or not, and no way to guarentee that the behaviour > > will stay the same in future. > > > > The multicall dispatches to the regular hypercall handlers, which > > (cant?) > > They can - current->mc_state.flags has _MCSF_in_multicall > set. > > > and certainly shouldn't distinguish between a regular hypercall > > and multicall. > > I agree with this. Yet it's a bug in the caller to request no > preemption at this layer for a constituent hypercall that can itself > preempt. But that's only a problem for the caller, not for the > hypervisor. > > > As I have been looking through this code, I have noticed that the NDEBUG > > parameter corruption will break half of our existing preemption logic, > > which does use some of the parameters to hold preemption information. > > Certainly not - call_list is being copied over a second time a few > lines after that NDEBUG section. > > Jan > Clear, thank you two for your discussion. So the only thing need to be done here is fixing the potential security issue, right? I will follow this. Thanks, Chao > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel