linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mateusz Guzik <mjguzik@gmail.com>
To: brauner@kernel.org
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,
	Mateusz Guzik <mjguzik@gmail.com>
Subject: [HACK PATCH] fs: dodge atomic in putname if ref == 1
Date: Tue,  4 Jun 2024 15:24:48 +0200	[thread overview]
Message-ID: <20240604132448.101183-1-mjguzik@gmail.com> (raw)

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).

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


             reply	other threads:[~2024-06-04 13:24 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-04 13:24 Mateusz Guzik [this message]
2024-06-05 15:20 ` [HACK PATCH] fs: dodge atomic in putname if ref == 1 Christian Brauner
2024-06-05 15:23   ` Mateusz Guzik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240604132448.101183-1-mjguzik@gmail.com \
    --to=mjguzik@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=brauner@kernel.org \
    --cc=daclash@linux.microsoft.com \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).