Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Thu, 14 Dec 2023 14:44:28 +0000
From:   =?iso-8859-1?Q?Lu=EDs?= Henriques <lhenriques@suse.de>
To:     David Howells <dhowells@redhat.com>
Cc:     Eric Biggers <ebiggers@kernel.org>,
        Jarkko Sakkinen <jarkko@kernel.org>, keyrings@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] keys: flush work when accessing /proc/key-users
Message-ID: <ZXsUzBRR2uc81FK0@suse.de>
References: <20231207024323.GA1994@sol.localdomain>
 <20231206145744.17277-1-lhenriques@suse.de>
 <498294.1701878642@warthog.procyon.org.uk>
 <87bkb3z047.fsf@suse.de>
 <2744563.1702303367@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2744563.1702303367@warthog.procyon.org.uk>
Precedence: bulk

Hi David,

On Mon, Dec 11, 2023 at 02:02:47PM +0000, David Howells wrote:
<snip>
> > However, that would only fix the flakiness of the key quota for fs/crypto/,
> > not for other users of the keyrings service.  Maybe this suggests that
> > key_put() should release the key's quota right away if the key's refcount
> > drops to 0?
> 
> That I would be okay with as the key should be removed in short order.
> 
> Note that you'd have to change the spinlocks on key->user->lock to irq-locking
> types.  Or maybe we can do without them, at least for key gc, and use atomic
> counters.  key_invalidate() should probably drop the quota also.

I was trying to help with this but, first, I don't think atomic counters
would actually be a solution.  For example, we have the following in
key_alloc():

	spin_lock(&user->lock);
	if (!(flags & KEY_ALLOC_QUOTA_OVERRUN)) {
		if (user->qnkeys + 1 > maxkeys ||
		    user->qnbytes + quotalen > maxbytes ||
		    user->qnbytes + quotalen < user->qnbytes)
			goto no_quota;
	}
	user->qnkeys++;
	user->qnbytes += quotalen;
	spin_unlock(&user->lock);

Thus, I don't think it's really possible to simply stop using a lock
without making these checks+changes non-atomic.

As for using spin_lock_irq() or spin_lock_irqsave(), my understanding is
that the only places where this could be necessary is in key_put() and,
possibly, key_payload_reserve().  key_alloc() shouldn't need that.

Finally, why would key_invalidate() require handling quotas?  I'm probably
just missing some subtlety, but I don't see the user->usage refcount being
decremented anywhere in that path (or anywhere else, really).

Cheers,
--
Lu?s