Re: [PATCH] net: qrtr: ns: Fix module refcnt

[Manivannan Sadhasivam]

+ 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>
> 

-- 
மணிவண்ணன் சதாசிவம்