Messages in this thread | | | Date | Mon, 15 Mar 2021 15:02:58 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v8 3/8] Use atomic_t for ucounts reference counting |
| |
On Wed, Mar 10, 2021 at 01:01:28PM +0100, Alexey Gladkov wrote: > The current implementation of the ucounts reference counter requires the > use of spin_lock. We're going to use get_ucounts() in more performance > critical areas like a handling of RLIMIT_SIGPENDING.
This really looks like it should be refcount_t. I read the earlier thread[1] on this, and it's not clear to me that this is a "normal" condition. I think there was a bug in that version (This appeared to *instantly* crash at boot with mnt_init() calling alloc_mnt_ns() calling inc_ucount()). The current code looks like just a "regular" reference counter of the allocated struct ucounts. Overflow should be very unexpected, yes? And operating on a "0" ucounts should be a bug too, right?
> [...] > +/* 127: arbitrary random number, small enough to assemble well */ > +#define refcount_zero_or_close_to_overflow(ucounts) \ > + ((unsigned int) atomic_read(&ucounts->count) + 127u <= 127u)
Regardless, this should absolutely not have "refcount" as a prefix. I realize it's only used here, but that's needlessly confusing with regard to it being atomic_t not refcount_t.
> +struct ucounts *get_ucounts(struct ucounts *ucounts) > +{ > + if (ucounts) { > + if (refcount_zero_or_close_to_overflow(ucounts)) { > + WARN_ONCE(1, "ucounts: counter has reached its maximum value"); > + return NULL; > + } > + atomic_inc(&ucounts->count); > + } > + return ucounts; > +}
I feel like this should just be:
refcount_inc_not_zero(&ucounts->count);
Or, to address Linus's comment in the v3 series, change get_ucounts to not return NULL first -- I can't see why that can ever happen in v8.
-Kees
[1] https://lore.kernel.org/lkml/116c7669744404364651e3b380db2d82bb23f983.1610722473.git.gladkov.alexey@gmail.com/
-- Kees Cook
| |