From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [PATCH net-next] net: sched: change tcf_del_walker() to use concurrent-safe delete Date: Mon, 03 Sep 2018 23:33:26 +0300 Message-ID: References: <1535958361-6778-1-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Linux Kernel Network Developers , Jamal Hadi Salim , Jiri Pirko , David Miller To: Cong Wang Return-path: Received: from mail-eopbgr20072.outbound.protection.outlook.com ([40.107.2.72]:24304 "EHLO EUR02-VE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727552AbeIDAzh (ORCPT ); Mon, 3 Sep 2018 20:55:37 -0400 In-reply-to: Sender: netdev-owner@vger.kernel.org List-ID: On Mon 03 Sep 2018 at 18:50, Cong Wang wrote: > On Mon, Sep 3, 2018 at 12:06 AM Vlad Buslov wrote: >> >> Action API was changed to work with actions and action_idr in concurrency >> safe manner, however tcf_del_walker() still uses actions without taking >> reference to them first and deletes them directly, disregarding possible >> concurrent delete. >> >> Change tcf_del_walker() to use tcf_idr_delete_index() that doesn't require >> caller to hold reference to action and accepts action id as argument, >> instead of direct action pointer. > > Hmm, why doesn't tcf_del_walker() just take idrinfo->lock? At least > tcf_dump_walker() already does. Because tcf_del_walker() calls __tcf_idr_release(), which take idrinfo->lock itself (deadlock). It also calls sleeping functions like tcf_action_goto_chain_fini(), so just implementing function that releases action without taking idrinfo->lock is not enough.