Re: [PATCH v3 3/4] ibacm: Fix a retry loop calculation race condition

Jason Gunthorpe
2017-11-14T23:55:22Z

On Tue, Nov 14, 2017 at 11:43:47AM -0500, Michael J. Ruhl wrote:

> diff --git a/ibacm/prov/acmp/src/acmp.c b/ibacm/prov/acmp/src/acmp.c
> index d707b8e..884fc48 100644
> +++ b/ibacm/prov/acmp/src/acmp.c
> @@ -1579,10 +1579,12 @@ static void *acmp_retry_handler(void *context)
> pthread_mutex_unlock(&acmp_dev_lock);
>
> acmp_process_timeouts();
> - wait = (int) (next_expire - time_stamp_ms());
> - if (wait > 0 && atomic_get(&wait_cnt)) {
> - pthread_testcancel();
> - event_wait(&timeout_event, wait);
> + if (next_expire != -1) {
> + wait = (int) (next_expire - time_stamp_ms());

This (int) cast is also wrong..

If you want to do signed subtraction then you need to cast the
arguments to '-' not the result, and since time_stamp_ms() is u64,
casting it to int will truncate.

wait needs to be int64_t, next_expire should be int64_t and then no
casts are needed.

All of time_stamp_* should return int64_t

And the #define time_stamp_* should not be macros, but inline
functions returning int64_t to make the return type guarenteed and
clear.

This isn't really related to this series, but if you send a followup
patch it would be nice since it has been now been noticed :)

Jason