Re: [Xen-devel] [PATCH for-4.10] libs/evtchn: Remove active handler on clean-up or failure

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Wei,

On 14/11/17 13:53, Wei Liu wrote:
> On Tue, Nov 14, 2017 at 12:14:14PM +0000, Julien Grall wrote:
> > Hi,
> >
> > On 14/11/17 11:51, Ian Jackson wrote:
> > > Ross Lagerwall writes ("Re: [PATCH for-4.10] libs/evtchn: Remove active handler on 
> > > clean-up or failure"):
> > > > On 11/10/2017 05:10 PM, Julien Grall wrote:
> > > > > Commit 89d55473ed16543044a31d1e0d4660cf5a3f49df "xentoolcore_restrict_all:
> > > > > Implement for libxenevtchn" added a call to register allowing to
> > > > > restrict the event channel.
> > > > >
> > > > > However, the call to deregister the handler was not performed if open
> > > > > failed or when closing the event channel. This will result to corrupt
> > > > > the list of handlers and potentially crash the application later one.
> > > Sorry for not spotting this during review.
> > > The fix is correct as far as it goes, so:
> > >
> > > Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > >
> > > > > The call to xentoolcore_deregister_active_handle is done at the same
> > > > > place as for the grants. But I am not convinced this is thread safe as
> > > > > there are potential race between close the event channel and restict
> > > > > handler. Do we care about that?
> > > ...
> > > > However, I think it should call xentoolcore__deregister_active_handle()
> > > > _before_ calling osdep_evtchn_close() to avoid trying to restrict a
> > > > closed fd or some other fd that happens to have the same number.
> > > You are right.  But this slightly weakens the guarantee provided by
> > > xentoolcore_restrict_all.
> > >
> > > > I think all the other libs need to be fixed as well, unless there was a
> > > > reason it was done this way.
> > > I will send a further patch.  In the meantime I suggest we apply
> > > Julien's fix.
> > I am going to leave the decision to you and Wei. It feels a bit odd to
> > release-ack my patch :).
> We can only commit patches that are both acked and release-acked. The
> latter gives RM control over when the patch should be applied.
> Sometimes it is better to wait until something else happens (like
> getting the tree to a stable state).
>
> That's how I used release-ack anyway.
I feel a bit odd to release-ack my patch and usually for Arm patches 
deferred to Stefano the decision whether the patch is suitable for the 
release.
> For this particular patch, my interpretation of what you just said
> is you've given us release-ack and we can apply this patch anytime. I
> will commit it soon.
Thanks! I hope it will fixed some osstest failure.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel