From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v2] timer: fix race condition
Date: Wed, 19 Dec 2018 19:54:15 +0100
Message-ID: <1710260.bNaUKXAX4B@xps>
References: <1543517626-142526-1-git-send-email-erik.g.carrillo@intel.com>
 <1545235774-9834-1-git-send-email-erik.g.carrillo@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, Gavin.Hu@arm.com, rsanford@akamai.com, olivier.matz@6wind.com,
 stephen@networkplumber.org, bruce.richardson@intel.com, stable@dpdk.org
To: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Return-path: <dev-bounces@dpdk.org>
In-Reply-To: <1545235774-9834-1-git-send-email-erik.g.carrillo@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

19/12/2018 17:09, Erik Gabriel Carrillo:
> rte_timer_manage() adds expired timers to a "run list", and walks the
> list, transitioning each timer from the PENDING to the RUNNING state.
> If another lcore resets or stops the timer at precisely this
> moment, the timer state would instead be set to CONFIG by that other
> lcore, which would cause timer_manage() to skip over it. This is
> expected behavior.
> 
> However, if a timer expires quickly enough, there exists the
> following race condition that causes the timer_manage() routine to
> misinterpret a timer in CONFIG state, resulting in lost timers:
> 
> - Thread A:
>   - starts a timer with rte_timer_reset()
>   - the timer is moved to CONFIG state
>   - the spinlock associated with the appropriate skiplist is acquired
>   - timer is inserted into the skiplist
>   - the spinlock is released
> - Thread B:
>   - executes rte_timer_manage()
>   - find above timer as expired, add it to run list
>   - walk run list, see above timer still in CONFIG state, unlink it from
>     run list and continue on
> - Thread A:
>   - move timer to PENDING state
>   - return from rte_timer_reset()
>   - timer is now in PENDING state, but not actually linked into a
>     pending list or a run list and will never get processed further
>     by rte_timer_manage()
> 
> This commit fixes this race condition by only releasing the spinlock
> after the timer state has been transitioned from CONFIG to PENDING,
> which prevents rte_timer_manage() from seeing an incorrect state.
> 
> Fixes: 9b15ba895b9f ("timer: use a skip list")
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>

+Cc: stable@dpdk.org

Applied, thanks