Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
References: <20210328064555.93365-1-memxor@gmail.com>
User-agent: mu4e 1.4.10; emacs 27.1
From:   Vlad Buslov <vladbu@nvidia.com>
To:     Kumar Kartikeya Dwivedi <memxor@gmail.com>
CC:     <netdev@vger.kernel.org>, Vlad Buslov <vladbu@mellanox.com>,
        Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] net: sched: extend lifetime of new action in
 replace mode
In-Reply-To: <20210328064555.93365-1-memxor@gmail.com>
Date:   Mon, 29 Mar 2021 12:05:12 +0300
Message-ID: <ygnhk0pqgwjb.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Mar 2021 09:05:19.1728
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 5dbfe19e-0b9e-4094-a231-08d8f291c6d2
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT053.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4344
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Kumar,

Thanks for the patch!

On Sun 28 Mar 2021 at 09:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> When creating an action in replace mode, in tcf_action_add, the refcount
> of existing actions is rightly raised during tcf_idr_check_alloc call,
> but for new actions a dummy placeholder entry is created. This is then
> replaced with the actual action during tcf_idr_insert_many, but between
> this and the tcf_action_put_many call made in replace mode, we don't
> hold a reference to the newly created action while it has been
> published. This means that it can disappear under our feet, and that
> newly created actions are destroyed right after their creation as their
> refcount drops to 0 in replace mode.

Could you describe the reproduction in more details? Looking at the code
it seems that there are two ways actions are overwritten/deleted:

1. Directly through action API, which is still serialized by rtnl lock.

2. Classifier API, which doesn't use rtnl lock anymore and can execute
concurrently.

Actions created by path 2 also have their bind count incremented which
prevents them from being deleted by path 1 and cls API can only deleted
them together with classifier that points to them.

>
> This leads to commands like tc action replace reporting success in
> creation of the action and just removing them right after adding the
> action, leading to confusion in userspace.
>
> Fix this by raising the refcount of a newly created action and then
> pairing that increment with a tcf_action_put call to release the
> reference held during insertion. This is only needed in replace mode.
> We use a relaxed store as insert will ensure its visibility.
>
> Fixes: cae422f379f3 ("net: sched: use reference counting action init")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/net/act_api.h | 1 +
>  net/sched/act_api.c   | 5 ++++-
>  net/sched/cls_api.c   | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 2bf3092ae7ec..8b375b46cc04 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -181,6 +181,7 @@ int tcf_register_action(struct tc_action_ops *a, struct pernet_operations *ops);
>  int tcf_unregister_action(struct tc_action_ops *a,
>  			  struct pernet_operations *ops);
>  int tcf_action_destroy(struct tc_action *actions[], int bind);
> +void tcf_action_put_many(struct tc_action *actions[]);
>  int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
>  		    int nr_actions, struct tcf_result *res);
>  int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b919826939e0..7e26c18cdb15 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -778,7 +778,7 @@ static int tcf_action_put(struct tc_action *p)
>  }
>  
>  /* Put all actions in this array, skip those NULL's. */
> -static void tcf_action_put_many(struct tc_action *actions[])
> +void tcf_action_put_many(struct tc_action *actions[])
>  {
>  	int i;
>  
> @@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  	if (err != ACT_P_CREATED)
>  		module_put(a_o->owner);
>  
> +	if (err == ACT_P_CREATED && ovr)

So here you only take additional reference on newly created action...

> +		refcount_set(&a->tcfa_refcnt, 2);
> +
>  	return a;
>  
>  err_out:
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d3db70865d66..666077c23147 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3074,6 +3074,9 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
>  			exts->nr_actions = err;
>  		}
>  	}
> +
> +	if (ovr)

... but here you always release the reference if ovr is set, even when
overwriting existing action.

> +		tcf_action_put_many(exts->actions);

So, what happens here is actions were 'deleted' concurrently (their
tcfa_refcnt decremented by 1)? tcf_action_put_many() will decrement
refcnt again, it will reach 0, actions get actually deleted and
tcf_exts_validate() returns with non-error code, but exts->actions
pointing to freed memory? Doesn't look like the patches fixes the
described issue, unless I'm missing something.

>  #else
>  	if ((exts->action && tb[exts->action]) ||
>  	    (exts->police && tb[exts->police])) {