Received-SPF: pass (google.com: domain of linux-kernel+bounces-202819-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 5 Jun 2024 17:20:19 +0200
From: Christian Brauner <brauner@kernel.org>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: viro@zeniv.linux.org.uk, jack@suse.cz, linux-kernel@vger.kernel.org, 
	linux-fsdevel@vger.kernel.org, axboe@kernel.dk, daclash@linux.microsoft.com
Subject: Re: [HACK PATCH] fs: dodge atomic in putname if ref == 1
Message-ID: <20240605-zeitraffer-fachzeitschrift-e74730507b59@brauner>
References: <20240604132448.101183-1-mjguzik@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240604132448.101183-1-mjguzik@gmail.com>

On Tue, Jun 04, 2024 at 03:24:48PM +0200, Mateusz Guzik wrote:
> The struct used to be refcounted with regular inc/dec ops, atomic usage
> showed up in commit 03adc61edad4 ("audit,io_uring: io_uring openat
> triggers audit reference count underflow").
> 
> If putname spots a count of 1 there is no legitimate way of anyone to
> bump it and these modifications are low traffic (names are not heavily)
> shared, thus one can do a load first and if the value of 1 is found the
> atomic can be elided -- this is the last reference..
> 
> When performing a failed open this reduces putname on the profile from
> ~1.60% to ~0.2% and bumps the syscall rate by just shy of 1% (the
> discrepancy is due to now bigger stalls elsewhere).

I suspect you haven't turned audit on in general because that would give
you performance impact in a bunch of places. Can't we just do something
where we e.g., use plain refcounts if audit isn't turned on?
(audit_dummy_context() or whatever it's called).

> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> This is a lazy hack.
> 
> The race is only possible with io_uring which has a dedicated entry
> point, thus a getname variant which takes it into account could store
> the need to use atomics as a flag in struct filename. To that end
> getname could take a boolean indicating this, fronted with some inlines
> and the current entry point renamed to __getname_flags to hide it.
> 
> Option B is to add a routine which "upgrades" to atomics after getname
> returns, but that's a littly fishy vs audit_reusename.
> 
> At the end of the day all spots which modify the ref could branch on the
> atomics flag.
> 
> I opted to not do it since the hack below undoes the problem for me.
> 
> I'm not going to fight for this hack though, it is merely a placeholder
> until someone(tm) fixes things.
> 
> If the hack is considered a no-go and the appraoch described above is
> considered fine, I can submit a patch some time this month to sort it
> out, provided someone tells me how to name a routine which grabs a ref
> -- the op is currently opencoded and "getname" allocates instead of
> merely refing. would "refname" do it?
> 
>  fs/namei.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 37fb0a8aa09a..f9440bdb21d0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -260,11 +260,13 @@ void putname(struct filename *name)
>  	if (IS_ERR(name))
>  		return;
>  
> -	if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
> -		return;
> +	if (unlikely(atomic_read(&name->refcnt) != 1)) {
> +		if (WARN_ON_ONCE(!atomic_read(&name->refcnt)))
> +			return;
>  
> -	if (!atomic_dec_and_test(&name->refcnt))
> -		return;
> +		if (!atomic_dec_and_test(&name->refcnt))
> +			return;
> +	}
>  
>  	if (name->name != name->iname) {
>  		__putname(name->name);
> -- 
> 2.39.2
>