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

[Ross Lagerwall <ross.lagerwall@citrix.com>]

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.
> 
> Fix it by calling xentoolcore_deregister_active_handle on failure and
> closure.

Thanks for fixing this.

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
> 
> This patch is fixing a bug introduced after the code freeze by
> "xentoolcore_restrict_all: Implement for libxenevtchn".
> 
> 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?

Both xentoolcore__deregister_active_handle() and 
xentoolcore_restrict_all() hold the same lock when mutating the list so 
there shouldn't be a problem with the list itself.

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.

I think all the other libs need to be fixed as well, unless there was a 
reason it was done this way.

-- 
Ross Lagerwall

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