net: qrtr: ns: Fix module refcnt

Commit Message

Chris Lew May 13, 2024, 5:31 p.m. UTC

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 <[e-mail]>
Tested-by: Jeffrey Hugo <[e-mail]>
Signed-off-by: Chris Lew <[e-mail]>
---
This patch takes heavy influence from the following TIPC patch.

Link: https://lore.kernel.org/all/[e-mail]/
---
 net/qrtr/ns.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)


---
base-commit: e7b4ef8fffaca247809337bb78daceb406659f2d
change-id: 20240508-fix-qrtr-rmmod-5265be704bad

Best regards,

Comments

Manivannan Sadhasivam May 14, 2024, 8:43 a.m. UTC | #1

+ 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 <[e-mail]>
> Tested-by: Jeffrey Hugo <[e-mail]>
> Signed-off-by: Chris Lew <[e-mail]>

Reviewed-by: Manivannan Sadhasivam <[e-mail]>

- Mani

> ---
> This patch takes heavy influence from the following TIPC patch.
> 
> Link: https://lore.kernel.org/all/[e-mail]/
> ---
>  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 <[e-mail]>
>

Jeffrey Hugo May 14, 2024, 3:05 p.m. UTC | #2

On 5/13/2024 11:31 AM, 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 <[e-mail]>
> Tested-by: Jeffrey Hugo <[e-mail]>
> Signed-off-by: Chris Lew <[e-mail]>

Reviewed-by: Jeffrey Hugo <[e-mail]>

[e-mail] May 16, 2024, 8:50 a.m. UTC | #3

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <[e-mail]>:

On Mon, 13 May 2024 10:31:46 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - net: qrtr: ns: Fix module refcnt
    https://git.kernel.org/netdev/net/c/fd76e5ccc48f

You are awesome, thank you!

Patch

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);