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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
>
On Wed, Jun 5, 2024 at 5:20 PM Christian Brauner <[email protected]> wrote:
>
> 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).
>
That would still give atomics for audit users which don't play with io_uring.
The part below --- describes one idea what to do with this.
> >
> > Signed-off-by: Mateusz Guzik <[email protected]>
> > ---
> >
> > 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
> >
--
Mateusz Guzik <mjguzik gmail.com>