From: ebiederm@xmission.com (Eric W. Biederman)
To: Alexey Gladkov <gladkov.alexey@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>, Kees Cook <keescook@chromium.org>,
Kernel Hardening <kernel-hardening@lists.openwall.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Linux Containers <containers@lists.linux-foundation.org>,
Jann Horn <jannh@google.com>, LKML <linux-kernel@vger.kernel.org>,
Oleg Nesterov <oleg@redhat.com>,
linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
Alexey Gladkov <legion@kernel.org>
Subject: Re: [PATCH v9 3/8] Use atomic_t for ucounts reference counting
Date: Mon, 05 Apr 2021 12:01:41 -0500 [thread overview]
Message-ID: <m1eefoll6y.fsf@fess.ebiederm.org> (raw)
In-Reply-To: <54956fd06ab4a9938421f345ecf2e1518161cb38.1616533074.git.gladkov.alexey@gmail.com> (Alexey Gladkov's message of "Tue, 23 Mar 2021 21:59:12 +0100")
Alexey Gladkov <gladkov.alexey@gmail.com> writes:
> 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.
>
> Now we need to use spin_lock only if we want to change the hashtable.
>
> v9:
> * Use a negative value to check that the ucounts->count is close to
> overflow.
Overall this looks good, one small issue below.
Eric
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index 50cc1dfb7d28..7bac19bb3f1e 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -11,7 +11,7 @@
> struct ucounts init_ucounts = {
> .ns = &init_user_ns,
> .uid = GLOBAL_ROOT_UID,
> - .count = 1,
> + .count = ATOMIC_INIT(1),
> };
>
> #define UCOUNTS_HASHTABLE_BITS 10
> @@ -139,6 +139,15 @@ static void hlist_add_ucounts(struct ucounts *ucounts)
> spin_unlock_irq(&ucounts_lock);
> }
>
> +struct ucounts *get_ucounts(struct ucounts *ucounts)
> +{
> + if (ucounts && atomic_add_negative(1, &ucounts->count)) {
> + atomic_dec(&ucounts->count);
^^^^^^^^^^^^^^^^^^^^^^^^^^^
To handle the pathological case of all of the other uses calling
put_ucounts after the value goes negative, the above should
be put_ucounts intead of atomic_dec.
> + ucounts = NULL;
> + }
> + return ucounts;
> +}
> +
> struct ucounts *alloc_ucounts(struct user_namespace *ns, kuid_t uid)
> {
> struct hlist_head *hashent = ucounts_hashentry(ns, uid);
> @@ -155,7 +164,7 @@ struct ucounts *alloc_ucounts(struct user_namespace *ns, kuid_t uid)
>
> new->ns = ns;
> new->uid = uid;
> - new->count = 0;
> + atomic_set(&new->count, 1);
>
> spin_lock_irq(&ucounts_lock);
> ucounts = find_ucounts(ns, uid, hashent);
> @@ -163,33 +172,12 @@ struct ucounts *alloc_ucounts(struct user_namespace *ns, kuid_t uid)
> kfree(new);
> } else {
> hlist_add_head(&new->node, hashent);
> - ucounts = new;
> + spin_unlock_irq(&ucounts_lock);
> + return new;
> }
> }
> - if (ucounts->count == INT_MAX)
> - ucounts = NULL;
> - else
> - ucounts->count += 1;
> spin_unlock_irq(&ucounts_lock);
> - return ucounts;
> -}
> -
> -struct ucounts *get_ucounts(struct ucounts *ucounts)
> -{
> - unsigned long flags;
> -
> - if (!ucounts)
> - return NULL;
> -
> - spin_lock_irqsave(&ucounts_lock, flags);
> - if (ucounts->count == INT_MAX) {
> - WARN_ONCE(1, "ucounts: counter has reached its maximum value");
> - ucounts = NULL;
> - } else {
> - ucounts->count += 1;
> - }
> - spin_unlock_irqrestore(&ucounts_lock, flags);
> -
> + ucounts = get_ucounts(ucounts);
> return ucounts;
> }
>
> @@ -197,15 +185,12 @@ void put_ucounts(struct ucounts *ucounts)
> {
> unsigned long flags;
>
> - spin_lock_irqsave(&ucounts_lock, flags);
> - ucounts->count -= 1;
> - if (!ucounts->count)
> + if (atomic_dec_and_test(&ucounts->count)) {
> + spin_lock_irqsave(&ucounts_lock, flags);
> hlist_del_init(&ucounts->node);
> - else
> - ucounts = NULL;
> - spin_unlock_irqrestore(&ucounts_lock, flags);
> -
> - kfree(ucounts);
> + spin_unlock_irqrestore(&ucounts_lock, flags);
> + kfree(ucounts);
> + }
> }
>
> static inline bool atomic_long_inc_below(atomic_long_t *v, int u)
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
next prev parent reply other threads:[~2021-04-05 17:01 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-23 20:59 [PATCH v9 0/8] Count rlimits in each user namespace Alexey Gladkov
2021-03-23 20:59 ` [PATCH v9 1/8] Increase size of ucounts to atomic_long_t Alexey Gladkov
2021-03-23 20:59 ` [PATCH v9 2/8] Add a reference to ucounts for each cred Alexey Gladkov
2021-03-23 20:59 ` [PATCH v9 3/8] Use atomic_t for ucounts reference counting Alexey Gladkov
2021-04-05 17:01 ` Eric W. Biederman [this message]
2021-03-23 20:59 ` [PATCH v9 4/8] Reimplement RLIMIT_NPROC on top of ucounts Alexey Gladkov
2021-04-05 16:56 ` Eric W. Biederman
2021-04-06 15:44 ` Alexey Gladkov
2021-04-07 16:56 ` Eric W. Biederman
2021-03-23 20:59 ` [PATCH v9 5/8] Reimplement RLIMIT_MSGQUEUE " Alexey Gladkov
2021-03-23 20:59 ` [PATCH v9 6/8] Reimplement RLIMIT_SIGPENDING " Alexey Gladkov
2021-04-05 16:58 ` Eric W. Biederman
2021-03-23 20:59 ` [PATCH v9 7/8] Reimplement RLIMIT_MEMLOCK " Alexey Gladkov
2021-03-23 20:59 ` [PATCH v9 8/8] kselftests: Add test to check for rlimit changes in different user namespaces Alexey Gladkov
2021-04-05 17:03 ` [PATCH v9 0/8] Count rlimits in each user namespace Eric W. Biederman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m1eefoll6y.fsf@fess.ebiederm.org \
--to=ebiederm@xmission.com \
--cc=akpm@linux-foundation.org \
--cc=axboe@kernel.dk \
--cc=containers@lists.linux-foundation.org \
--cc=gladkov.alexey@gmail.com \
--cc=jannh@google.com \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=legion@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=oleg@redhat.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).