+ Qiang (who also reported a similar issue)
On Mon, May 13, 2024 at 10:31:46AM -0700, Chris Lew wrote:
> The qrtr protocol core logic and the qrtr nameservice are combined into
> a single module. Neither the core logic or nameservice provide much
> functionality by themselves; combining the two into a single module also
> prevents any possible issues that may stem from client modules loading
> inbetween qrtr and the ns.
>
> Creating a socket takes two references to the module that owns the
> socket protocol. Since the ns needs to create the control socket, this
> creates a scenario where there are always two references to the qrtr
> module. This prevents the execution of 'rmmod' for qrtr.
>
> To resolve this, forcefully put the module refcount for the socket
> opened by the nameservice.
>
> Fixes: a365023a76f2 ("net: qrtr: combine nameservice into main module")
> Reported-by: Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>
> Tested-by: Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>
> Signed-off-by: Chris Lew <quic_clew@xxxxxxxxxxx>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
- Mani
> ---
> This patch takes heavy influence from the following TIPC patch.
>
> Link: https://lore.kernel.org/all/1426642379-20503-2-git-send-email-ying.xue@xxxxxxxxxxxxx/
> ---
> net/qrtr/ns.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index abb0c70ffc8b..654a3cc0d347 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -725,6 +725,24 @@ int qrtr_ns_init(void)
> if (ret < 0)
> goto err_wq;
>
> + /* As the qrtr ns socket owner and creator is the same module, we have
> + * to decrease the qrtr module reference count to guarantee that it
> + * remains zero after the ns socket is created, otherwise, executing
> + * "rmmod" command is unable to make the qrtr module deleted after the
> + * qrtr module is inserted successfully.
> + *
> + * However, the reference count is increased twice in
> + * sock_create_kern(): one is to increase the reference count of owner
> + * of qrtr socket's proto_ops struct; another is to increment the
> + * reference count of owner of qrtr proto struct. Therefore, we must
> + * decrement the module reference count twice to ensure that it keeps
> + * zero after server's listening socket is created. Of course, we
> + * must bump the module reference count twice as well before the socket
> + * is closed.
> + */
> + module_put(qrtr_ns.sock->ops->owner);
> + module_put(qrtr_ns.sock->sk->sk_prot_creator->owner);
> +
> return 0;
>
> err_wq:
> @@ -739,6 +757,15 @@ void qrtr_ns_remove(void)
> {
> cancel_work_sync(&qrtr_ns.work);
> destroy_workqueue(qrtr_ns.workqueue);
> +
> + /* sock_release() expects the two references that were put during
> + * qrtr_ns_init(). This function is only called during module remove,
> + * so try_stop_module() has already set the refcnt to 0. Use
> + * __module_get() instead of try_module_get() to successfully take two
> + * references.
> + */
> + __module_get(qrtr_ns.sock->ops->owner);
> + __module_get(qrtr_ns.sock->sk->sk_prot_creator->owner);
> sock_release(qrtr_ns.sock);
> }
> EXPORT_SYMBOL_GPL(qrtr_ns_remove);
>
> ---
> base-commit: e7b4ef8fffaca247809337bb78daceb406659f2d
> change-id: 20240508-fix-qrtr-rmmod-5265be704bad
>
> Best regards,
> --
> Chris Lew <quic_clew@xxxxxxxxxxx>
>
--
மணிவண்ணன் சதாசிவம்
