Re: [PATCH v11 1/6] s390/cert_store: Pin the user session keyring

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

> s390/cert_store: Pin the user session keyring

TL;DR: Commit message only: the stable tag should be v6.6+, not v6.1+, and the
changelog's keyring-lifetime reason looks inaccurate. It also omits the new
ERR_PTR handling. The code change looks worthwhile.

>     s390/cert_store: Pin the user session keyring
>
>     The user session keyring stays alive only because the session keyring is
>     pinned by current_cred().
>
>     Pin the reference at:
>
>     1. find_cs_keyring()
>     2. cleanup_cs_keys()
>     3. create_cs_keyring()
>
>     Cc: David Howells <dhowells@redhat.com>
>     Cc: stable@vger.kernel.org # v6.1+
>     Fixes: 8cf57d7217c3 ("s390: add support for user-defined certificates")
>     Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

The stable tag says "# v6.1+", but the commit named in Fixes: is not in
v6.1.  8cf57d7217c3 is contained in v6.6 and not in v6.5, and
arch/s390/kernel/cert_store.c does not exist in v6.1 through v6.5, so there
is nothing to backport there.

The other patches in this series that fix the same commit use "# v6.6+".
Should this be:

  Cc: stable@vger.kernel.org # v6.6+

The changelog says the user session keyring stays alive only because the
session keyring is pinned by current_cred().  Is that accurate?

look_up_user_keyrings() links the user session keyring into the per-user_ns
register:

    ret = key_link(reg_keyring, session_keyring);

and get_user_register() notes that "the keyring is pinned by the user_ns".
That link is what normally keeps it alive.

cred->session_keyring is usually a different keyring, either a "_ses"
keyring from join_session_keyring() or pam_keyinit, or NULL.  It is the user
session keyring only when install_session_keyring(user_session) happened to
run for that process.

The change itself looks worthwhile, since the owner has KEY_USR_ALL on that
keyring and can revoke or invalidate it, after which keyring_gc() can drop
the register link while cert_store still uses the pointer.  Could the
changelog give that reason instead?

The changelog also does not mention that the three callers now handle an
ERR_PTR return from get_user_session_keyring().  Before this patch that
ERR_PTR was passed straight into keyring_search(), key_unlink() and
keyring_alloc(), which dereference keyring->type.  Could that be described
as well?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/43615900108