diff mbox

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

Message ID 20171114164337.24557.45231.stgit@phlsvslse11.ph.intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Michael J. Ruhl Nov. 14, 2017, 4:43 p.m. UTC 
From: Michael J. Ruhl <michael.j.ruhl@intel.com>

The retry loop calculation uses a conversion to int of an unsigned
64 bit number (next_expire) minus the current time to decide if
event_wait() should be called.  This calculation works correctly as
long as the next_expire value is not the default value (-1).

If the next_expire is the default value, periodically this subtraction
can result in a very large postive timeout value (days rather than
milliseconds).

For example:
next_expire  = 0xFFFFFFFFFFFFFFFF  (-1)
current_ms = 0x15f7db52146  (today's ms since 1970)

max_delay_ms = (int) next_expire - future_ms

future_ms  = 0x15f80000000  = max_delay_ms 2147483647
future_ms  = 0x16080000000  = max_delay_ms 2147483647

Converting max_delay_ms to days:
2147483647 / 1000 / 60 / 60 / 24 == 24 days

0xxx180000000 - 0xxx080000000 = 4294967296

every 48 days, this issue repeats

This calculation can occur if a wait_cnt is incremented and a message
expiration is handled so that next_expire is not updated.  If wait_cnt
is incremented before the wait calculation is done (the race condition),
event_wait() can be called with the potentially very large value.

If next_expire is not updated, do not do the wait calculation and
avoid the race condition.

Reported-by: Morys Grzegorz <grzegorz.morys@intel.com>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 ibacm/prov/acmp/src/acmp.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jason Gunthorpe Nov. 14, 2017, 11:55 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael J. Ruhl Nov. 15, 2017, 7:20 p.m. UTC | #2 
> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Jason Gunthorpe
> Sent: Tuesday, November 14, 2017 6:55 PM
> To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> Cc: linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v3 3/4] ibacm: Fix a retry loop calculation race condition
> 
> 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.

Hmm, I think that the truncate is what is happening when delay issue (caused by the race condition) occurs.
I need to explore this some more, your suggested change my obviate the issue.

> 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 :)

I will do this in a follow on patch, but need more time to test so it will not be in this series.

Thanks for the comments!

Mike


> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/ibacm/prov/acmp/src/acmp.c b/ibacm/prov/acmp/src/acmp.c
index d707b8e..884fc48 100644
--- a/ibacm/prov/acmp/src/acmp.c
+++ 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());
+			if (wait > 0 && atomic_get(&wait_cnt)) {
+				pthread_testcancel();
+				event_wait(&timeout_event, wait);
+			}
 		}
 	}