On Tue, Dec 12, 2023 at 8:29 PM Marco Elver <elver@xxxxxxxxxx> wrote:
>
> > - stack_depot_put(alloc_meta->aux_stack[1]);
> > + new_handle = kasan_save_stack(0, depot_flags);
> > +
> > + spin_lock_irqsave(&aux_lock, flags);
>
> This is a unnecessary global lock. What's the problem here? As far as
> I can understand a race is possible where we may end up with
> duplicated or lost stack handles.
Yes, this is the problem. And this leads to refcount underflows in the
stack depot code, as we fail to keep precise track of the stack
traces.
> Since storing this information is best effort anyway, and bugs are
> rare, a global lock protecting this is overkill.
>
> I'd just accept the racyness and use READ_ONCE() / WRITE_ONCE() just
> to make sure we don't tear any reads/writes and the depot handles are
> valid.
This will help with the potential tears but will not help with the
refcount issues.
> There are other more complex schemes [1], but I think they are
> overkill as well.
>
> [1]: Since a depot stack handle is just an u32, we can have a
>
> union {
> depot_stack_handle_t handles[2];
> atomic64_t atomic_handle;
> } aux_stack;
> (BUILD_BUG_ON somewhere if sizeof handles and atomic_handle mismatch.)
>
> Then in the code here create the same union and load atomic_handle.
> Swap handle[1] into handle[0] and write the new one in handles[1].
> Then do a cmpxchg loop to store the new atomic_handle.
This approach should work. If you prefer, I can do this instead of a spinlock.
But we do need some kind of atomicity while rotating the aux handles
to make sure nothing gets lost.
Thanks!
