Re: [PATCH net v5] kcm: reject attaching sockets with a ULP, and TLS on KCM sockets

[Rishikesh Jethwani]

On Tue, Nov 3, 2026 at 2:05 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The commit message understates one user-visible regression.
> - [Low] The commit message says 'rmmod kcm waits until such sockets are
>   destroyed'.
> - [Low] Documentation/networking/kcm.rst ('Attach transport sockets' /
>   'Unattach transport sockets', lines 144-190) is not updated for the…
>
> --
>
> kcm: reject attaching sockets with a ULP, and TLS on KCM sockets
>
> With this commit, kcm_attach() refuses TCP sockets that already have a
> ULP, and it now refuses raw IPPROTO_TCP sockets too. Attached sockets get
> an unregistered dummy kcm_ulp_ops, so tcp_set_ulp() refuses kTLS on them
> afterwards. The marker stays until the socket is destroyed, and it pins
> the kcm module.
>
> Link: https://lore.kernel.org/netdev/3108e934-ec00-458b-8720-450554b35b55@xxxxxxxxx/
>
> > Holding a ULP also has visible side effects: getsockopt(TCP_ULP) and
> > inet_diag report "kcm" on a socket that was ever attached,
> > sk_has_decrypt_user() is true so PSP RX association is refused after the
> > attach, and adding an attached socket to a sockmap fails with -EINVAL
> > rather than -EBUSY.
>
> [Severity: Medium]
>
> Is the sockmap effect really limited to attached sockets?
> SIOCKCMUNATTACH does not remove the kcm_ulp_ops marker, because
> kcm_unattach() only clears sk_user_data:
>
> net/kcm/kcmsock.c:kcm_unattach() {
>     ...
>         write_lock_bh(&csk->sk_callback_lock);
>         csk->sk_user_data = NULL;
>         WRITE_ONCE(csk->sk_data_ready, psock->save_data_ready);
>     ...
> }
>
> sk_psock_init() checks for a ULP before it looks at sk_user_data:
>
> net/core/skmsg.c:sk_psock_init() {
>     ...
>         if (sk_is_inet(sk) && inet_csk_has_ulp(sk)) {
>                 psock = ERR_PTR(-EINVAL);
>                 goto out;
>         }
>     ...
> }
>
> Before this patch, this sequence succeeded:
>
>   SIOCKCMATTACH -> SIOCKCMUNATTACH -> BPF_MAP_UPDATE_ELEM on a sockmap
>
> With this patch, the map update fails with -EINVAL for the rest of the
> socket's life.
>
> Could the stable-facing paragraph that lists the user-visible changes also
> say that a socket that was ever attached can no longer be added to a
> sockmap or sockhash?

Good catch, I'll list the sockmap case in the stable paragraph,
and I'll fix the earlier paragraph that said the -EINVAL only applied to
attached sockets.

> > listen() after a disconnect fails with -EINVAL, as
> > the dummy has no .clone, and rmmod kcm waits until such sockets are
> > destroyed.
>
> [Severity: Low]
>
> Is "waits" accurate here? Unloading does not block on a non-zero module
> refcount:
>
> kernel/module/main.c:try_stop_module() {
>         /* If it's not unused, quit unless we're forcing. */
>         if (try_release_module_ref(mod) != 0) {
>                 *forced = try_force_unload(flags);
>                 if (!(*forced))
>                         return -EWOULDBLOCK;
>         }
>     ...
> }
>
> The reference taken by __module_get() in kcm_attach() is only dropped by
> tcp_cleanup_ulp() when the TCP socket is destroyed. As long as any TCP
> socket that was once attached still exists, rmmod kcm fails at once with
> "Module kcm is in use". This holds even after every KCM socket is closed
> and every socket has been unattached.

Right, rmmod fails immediately rather than waiting. I'll reword it to say
it fails while any once-attached socket exists.

> > diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> > index a7f94d42036fb..1f8bcbb8b9c3b 100644
> > --- a/net/kcm/kcmsock.c
> > +++ b/net/kcm/kcmsock.c
> > @@ -30,11 +30,26 @@
>
> [ ... ]
>
> > +/* Not registered, so TCP_ULP can't select it. It only holds
> > + * icsk_ulp_ops so tcp_set_ulp() refuses other ULPs like kTLS. Like any
> > + * ULP it stays until the socket is destroyed, pinning this module, and
> > + * tcp_cleanup_ulp() releases it. Clearing it on a live socket would race
> > + * with lockless readers such as do_tcp_getsockopt() and tcp_diag.
> > + *
> > + * No .clone: children copy icsk_ulp_ops without a module reference, so
> > + * a socket that was once attached must not listen().
> > + */
> > +static const struct tcp_ulp_ops kcm_ulp_ops = {
> > +     .name = "kcm",
> > +     .owner = THIS_MODULE,
> > +};
>
> [Severity: Low]
>
> This isn't a bug, but should Documentation/networking/kcm.rst be updated
> as well? The "Attach transport sockets" section only says:
>
>   - fd: file descriptor for TCP socket being attached
>
> The "Unattach transport sockets" section says unattaching "is
> straightforward".
>
> After this patch, SIOCKCMATTACH refuses raw IPPROTO_TCP sockets and
> sockets that already have a ULP, both with -EOPNOTSUPP.
>
> The kcm marker also stays after SIOCKCMUNATTACH. For the rest of the
> socket's life:
>
>   - setting TCP_ULP fails with -EEXIST
>   - getsockopt(TCP_ULP) and inet_diag report "kcm"
>   - listen() fails with -EINVAL
>   - sockmap insertion fails with -EINVAL
>   - PSP RX association is refused

Yes. The next version updates kcm.rst.

> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102220310.12649-1-rjethwani%40purestorage.com

pw-bot: cr