Received-SPF: pass (google.com: domain of linux-kernel+bounces-178448-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 14 May 2024 10:43:03 +0200
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Chris Lew <quic_clew@quicinc.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Bjorn Andersson <andersson@kernel.org>, Luca Weiss <luca@z3ntu.xyz>,
	Manivannan Sadhasivam <mani@kernel.org>,
	linux-arm-msm@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, Jeffrey Hugo <quic_jhugo@quicinc.com>,
	quic_qianyu@quicinc.com
Subject: Re: [PATCH] net: qrtr: ns: Fix module refcnt
Message-ID: <20240514084303.GD2463@thinkpad>
References: <20240513-fix-qrtr-rmmod-v1-1-312a7cd2d571@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240513-fix-qrtr-rmmod-v1-1-312a7cd2d571@quicinc.com>

+ 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@quicinc.com>
> Tested-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- 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@windriver.com/
> ---
>  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@quicinc.com>
> 

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