| Message ID | 20171110171050.19836-1-julien.grall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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@eu.citrix.com>
> > 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.
Ian.
On 11/14/2017 11:51 AM, 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@eu.citrix.com> > >>> 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. > Now that I look at it, a similar scenario can happen during open. Since the handle is registered before it is actually opened, a concurrent xentoolcore_restrict_all() will try to restrict a handle that it not properly set up. I think it is OK if xentoolcore_restrict_all() works with any open handle where a handle is defined as open if it has _completed_ the call to e.g. xenevtchn_open() and has not yet called xenevtchn_close().
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@eu.citrix.com> > >>> 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 :). Cheers,
Ross Lagerwall writes ("Re: [PATCH for-4.10] libs/evtchn: Remove active handler on clean-up or failure"):
> Now that I look at it, a similar scenario can happen during open. Since
> the handle is registered before it is actually opened, a concurrent
> xentoolcore_restrict_all() will try to restrict a handle that it not
> properly set up.
I think this is not a problem because the handle has thing->fd = -1.
So the restrict call will be a no-op (or give EBADF).
Ian.
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@eu.citrix.com> > > > > > > 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. 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.
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@eu.citrix.com> >>> >>>>> 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,
diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c index 14b7549a6b..2dba58bf00 100644 --- a/tools/libs/evtchn/core.c +++ b/tools/libs/evtchn/core.c @@ -56,6 +56,7 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags) err: osdep_evtchn_close(xce); + xentoolcore__deregister_active_handle(&xce->tc_ah); xtl_logger_destroy(xce->logger_tofree); free(xce); return NULL; @@ -69,6 +70,7 @@ int xenevtchn_close(xenevtchn_handle *xce) return 0; rc = osdep_evtchn_close(xce); + xentoolcore__deregister_active_handle(&xce->tc_ah); xtl_logger_destroy(xce->logger_tofree); free(xce); return rc;
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. 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? --- tools/libs/evtchn/core.c | 2 ++ 1 file changed, 2 insertions(+)