Message-ID: <CAHk-=wh4qK+zHrrYehidKRp4Fi6e4qUD6Tv6Ed8USxUC+H+HrQ@mail.gmail.com>
Date: Fri, 15 Mar 2024 10:10:00 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: David Teigland <teigland@...hat.com>
Cc: linux-kernel@...r.kernel.org, gfs2@...ts.linux.dev
Subject: Re: [GIT PULL] dlm fixes for 6.9

On Thu, 14 Mar 2024 at 11:43, David Teigland <teigland@...hat.com> wrote:
>
> Fix two refcounting bugs from recent changes:
> - misuse of atomic_dec_and_test results in missed ref decrement
> - wrong variable assignment results in another missed ref decrement

I pulled this, and then I unpulled it again.

That code is insane.

This is *NOT* sane or valid code:

+               while (atomic_read(&lkb->lkb_wait_count)) {
+                       if (atomic_dec_and_test(&lkb->lkb_wait_count))
+                               list_del_init(&lkb->lkb_wait_reply);
+
+                       unhold_lkb(lkb);
+               }

the above is completely crazy. That's simply not how atomics work.

What's the point of using a refcount - an atomic one at that - if you
just use it as a counter. That's not a "reference count", that's
literally just broken.

The whole - and *ONLY* - point of a refcount is that you are counting
references. References that *YOU* hold. Not that somebody else is
holding and you are releasing.

If you're the only holder of any counts, don't make them atomic, don't
put them in a data structure. But you're *not* the only holder fo that
refcount here, are you?

Using atomics for this kind of sequence shows some crazy crazy
behavior. It's not valid to say "ok, as long as this atomic is not
zero, let's decrement it and test if it's not zero".

Because for an atomic value to MAKE SENSE IN THE FIRST PLACE, there
could be somebody else that comes in and also possibly decrements it.

And if that happens between the test of "is this zero" and "did I
decrement it to zero", you now had two decrements, and that value is
now negative. So you didn't really have an atomic value, because you
did two operations on it.

And dammit, if that mutex means that it cannot happen, then WHY WAS IT
AN ATOMIC IN THE FIRST PLACE?

IOW, if you have locking that protects the value, then atomic accesses
are STILL wrong.

So there is not a single situation where I can see the above kind of
code ever being valid.

Now, if the issue is that you want to clean up something that is never
getting cleaned up by anybody else, and this is a fatal error, and
you're just trying to fix things up (badly), and you know that this is
all racy but the code is trying to kill a dead data structure, then
you should

 (a) need a damn big comment (bigger than the comment is already)

 (b) should *NOT* pretend to do some stupid "atomic decrement and test" loop

IOW, if what you want to do is get rid of stuck entries and set the
refcount to zero, then doing that would probably be something like

        /* This is broken, but.. */
        stale = atomic_xchg((&lkb->lkb_wait_count, 0);
        if (stale) {
                list_del_init(&lkb->lkb_wait_reply);
                do { unhold_lkb(lkb); } while (--stale);
        }

and it needs a much bigger comment than that "This is broken".

(And I don't know if you want that list_del_init() before or after the
'unhold N times' loop).

The above is still completely broken, but at least it doesn't do some
kind of odd non-atomic test and decrement stuff in a loop, and
hopefully makes it clear that we're very much talking about fixing up
stale final values

And no, I didn't look at the code around it. Because I really think
that "while (atomic_read(...)" loop cannot POSSIBLY be correct,
regardless of any context.

                Linus

Powered by blists - more mailing lists