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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 94733CD54BF for ; Mon, 25 Sep 2023 19:53:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229481AbjIYTxa (ORCPT ); Mon, 25 Sep 2023 15:53:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37766 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231853AbjIYTx3 (ORCPT ); Mon, 25 Sep 2023 15:53:29 -0400 Received: from Chamillionaire.breakpoint.cc (Chamillionaire.breakpoint.cc [IPv6:2a0a:51c0:0:237:300::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 42C1C109 for ; Mon, 25 Sep 2023 12:53:19 -0700 (PDT) Received: from fw by Chamillionaire.breakpoint.cc with local (Exim 4.92) (envelope-from ) id 1qkrdt-0000iR-1A; Mon, 25 Sep 2023 21:53:17 +0200 Date: Mon, 25 Sep 2023 21:53:17 +0200 From: Florian Westphal To: Pablo Neira Ayuso Cc: Florian Westphal , Phil Sutter , netfilter-devel@vger.kernel.org Subject: Re: [nf PATCH 2/5] netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests Message-ID: <20230925195317.GC22532@breakpoint.cc> References: <20230923013807.11398-1-phil@nwl.cc> <20230923013807.11398-3-phil@nwl.cc> <20230923110437.GB22532@breakpoint.cc> <20230923161813.GB19098@breakpoint.cc> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org Pablo Neira Ayuso wrote: > On Sat, Sep 23, 2023 at 06:18:13PM +0200, Florian Westphal wrote: > > callback_that_might_reset() > > { > > try_module_get ... > > rcu_read_unlock() > > mutex_lock(net->commit_mutex) > > dumper(); > > mutex_unlock(net->commit_mutex) > > rcu_read_lock(); > > module_put() > > } > > > > should do the trick. > > Idiom above LGTM, *except for net->commit_mutex*. Please do not use > ->commit_mutex: This will stall ruleset updates for no reason, netlink > dump would grab and release such mutex for each netlink_recvmsg() call > and netlink dump side will always retry because of NLM_F_EINTR. It will stall updates, but for good reason: we are making changes to the expressions state. We even emit AUDIT messages about this. So, I think the commit mutex is appropirate here. That said, if you totally disagree, then I suppose a new "reset" mutex could be used instead.