dlm fixes for 6.9

Hi Linus,

Please pull dlm fixes from tag:

git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git dlm-6.9

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

Thanks,
Dave

 fs/dlm/lock.c | 10 ++++++----
 fs/dlm/user.c | 10 +++++-----
 2 files changed, 11 insertions(+), 9 deletions(-)

Alexander Aring (2):
dlm: fix user space lkb refcounting
dlm: fix off-by-one waiters refcount handling

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

Yes, that looks pretty messed up, the counter should not be an atomic_t. I was a bit wary of making that atomic when it wasn't necessary, but didn't push back enough on that change:

commit 75a7d60134ce84209f2c61ec4619ee543aa8f466
Author: Alexander Aring <[email protected]>
Date: Mon May 29 17:44:38 2023 -0400

Currently the lkb_wait_count is locked by the rsb lock and it should be fine to handle lkb_wait_count as non atomic_t value. However for the overall process of reducing locking this patch converts it to an atomic_t value.

... and the result is the primitives get abused, and the code becomes crazy. My initial plan is to go back to a non-atomic counter there. It is indeed a recovery situation that involves a forced reset of state, but I'll need to go back and study that case further before I can say what it should finally look like. Whatever that looks like, it'll have a very good comment :) Dropping the pull is fine, there's a chance I may resend with the other patch and a new fix, we'll see.

Thanks,
Dave