From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 652D4C43381 for ; Tue, 19 Feb 2019 16:50:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 36FCA21773 for ; Tue, 19 Feb 2019 16:50:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729097AbfBSQut (ORCPT ); Tue, 19 Feb 2019 11:50:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42956 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728201AbfBSQut (ORCPT ); Tue, 19 Feb 2019 11:50:49 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 182C681F18; Tue, 19 Feb 2019 16:50:49 +0000 (UTC) Received: from localhost.localdomain (unknown [10.32.181.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8103017A65; Tue, 19 Feb 2019 16:50:47 +0000 (UTC) Message-ID: Subject: Re: [PATCH RFC 2/5] net/sched: prepare TC actions to properly validate the control action From: Davide Caratti To: Vlad Buslov Cc: Jamal Hadi Salim , Cong Wang , Jiri Pirko , "David S. Miller" , Paolo Abeni , "netdev@vger.kernel.org" In-Reply-To: References: Organization: red hat Content-Type: text/plain; charset="UTF-8" Date: Tue, 19 Feb 2019 17:50:46 +0100 Mime-Version: 1.0 User-Agent: Evolution 3.30.3 (3.30.3-1.fc29) Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 19 Feb 2019 16:50:49 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org hi Vlad, On Mon, 2019-02-18 at 16:52 +0000, Vlad Buslov wrote: > Hi Davide, > > I really like the idea of putting all action validation code in single > place, instead of spreading it between act API and specific actions init > code. sure, the previous validation code was in tcf_action_add_1(), and I attempted to fix the problems (well, it's the same problem showing up in 3 different ways) without adding the same code everywhere. Sadly, we need to handle correctly the situations where tcf_action_check_ctrlact() returns an error, and I'm seeing that we can't fix every action with the same exact code everywhere (to avoid leaking IDRs, or memory, or both). > See my comment regarding minor problem with using > tcf_action_set_ctrlact() on current net-next below. > > +void tcf_action_set_ctrlact(struct tc_action *p, int action, > > + struct tcf_chain *goto_chain) > > +{ > > + struct tcf_chain *old; > > + > > + old = xchg(&p->goto_chain, goto_chain); > > + if (old) > > + tcf_chain_put_by_act(old); > > This is blocking in current net-next because tcf_chain_put_by_act() > obtains block->lock mutex, so you can't call it while holding tcf_lock > spinlock like you do in following patches. Its not a big problem but > I guess you will have to just inline this code into all init handlers > instead of tcf_action_set_ctrlact() function. Argh! you are right. Thanks a lot for spotting this. Ok, let's kill tcf_action_set_ctrlact() and swap 'newchain' with a->goto_chain in each action init(), with a->tcfa_lock taken. I will then call tcf_chain_put_by_act() in the init() handler, after the spinlock is released. > BTW is atomic xchg strictly necessary if you hold tcf_lock while > re-assigning goto_chain pointer? No, it's not necessary. The new value of goto_chain is used only when the new value of tcfa_action is updated: so, if we just do the assignment of goto_chain and tcfa_action together with the spinlock taken, at least we are improving the current code. Thanks! -- davide