[v3] ax25: Fix refcount imbalance on inbound connections

Commit Message

Lars Kellogg-Stedman May 21, 2024, 6:23 p.m. UTC

From: Lars Kellogg-Stedman <[e-mail]>

When releasing a socket in ax25_release(), we call netdev_put() to
decrease the refcount on the associated ax.25 device. However, the
execution path for accepting an incoming connection never calls
netdev_hold(). This imbalance leads to refcount errors, and ultimately
to kernel crashes.

A typical call trace for the above situation looks like this:

    Call Trace:
    <TASK>
    ? show_regs+0x64/0x70
    ? __warn+0x83/0x120
    ? refcount_warn_saturate+0xb2/0x100
    ? report_bug+0x158/0x190
    ? prb_read_valid+0x20/0x30
    ? handle_bug+0x3e/0x70
    ? exc_invalid_op+0x1c/0x70
    ? asm_exc_invalid_op+0x1f/0x30
    ? refcount_warn_saturate+0xb2/0x100
    ? refcount_warn_saturate+0xb2/0x100
    ax25_release+0x2ad/0x360
    __sock_release+0x35/0xa0
    sock_close+0x19/0x20
    [...]

On reboot (or any attempt to remove the interface), the kernel gets
stuck in an infinite loop:

    unregister_netdevice: waiting for ax0 to become free. Usage count = 0

This patch corrects these issues by ensuring that we call netdev_hold()
and ax25_dev_hold() for new connections in ax25_accept().

Fixes: 7d8a3a477b
Signed-off-by: Lars Kellogg-Stedman <[e-mail]>
---
v3:
- Address naveenm's comments regarding the ordering of variable
  declarations (https://lore.kernel.org/netdev/[e-mail]/)

 net/ax25/af_ax25.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jakub Kicinski May 22, 2024, 5:07 p.m. UTC | #1

On Tue, 21 May 2024 14:23:25 -0400 [e-mail] wrote:
> Fixes: 7d8a3a477b

correct fixes tag for this hash would be:

Fixes: 7d8a3a477b3e ("ax25: Fix ax25 session cleanup problems")

Please post v4 as a new thread (not in reply to).
Please CC maintainers (per script/get_maintainer.pl)

Lars Kellogg-Stedman May 22, 2024, 6:25 p.m. UTC | #2

On Wed, May 22, 2024 at 10:07:01AM GMT, Jakub Kicinski wrote:
> correct fixes tag for this hash would be:
> 
> Fixes: 7d8a3a477b3e ("ax25: Fix ax25 session cleanup problems")

Jakub,

Thanks for the correction; I'll submit a new patch with a correct Fixes:
tag, but...

> Please CC maintainers (per script/get_maintainer.pl)

...the ax.25 tree is currently orphaned:

    AX.25 NETWORK LAYER
    L:	[e-mail]
    S:	Orphan
    W:	https://linux-ax25.in-berlin.de
    F:	include/net/ax25.h
    F:	include/uapi/linux/ax25.h
    F:	net/ax25/

Jakub Kicinski May 22, 2024, 6:54 p.m. UTC | #3

On Wed, 22 May 2024 14:25:36 -0400 Lars Kellogg-Stedman wrote:
> > Please CC maintainers (per script/get_maintainer.pl)  
> 
> ...the ax.25 tree is currently orphaned:
> 
>     AX.25 NETWORK LAYER
>     L:	[e-mail]
>     S:	Orphan
>     W:	https://linux-ax25.in-berlin.de
>     F:	include/net/ax25.h
>     F:	include/uapi/linux/ax25.h
>     F:	net/ax25/

Fair point, but get_maintainer acts in a bit of a hierarchical fashion,
so since we don't have AX25 maintainer and dedicated tree - the general
networking maintainers should get CCed

Patch

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 8077cf2ee44..d6f9fae06a9 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1378,8 +1378,10 @@  static int ax25_accept(struct socket *sock, struct socket *newsock,
 {
 	struct sk_buff *skb;
 	struct sock *newsk;
+	ax25_dev *ax25_dev;
 	DEFINE_WAIT(wait);
 	struct sock *sk;
+	ax25_cb *ax25;
 	int err = 0;
 
 	if (sock->state != SS_UNCONNECTED)
@@ -1434,6 +1436,10 @@  static int ax25_accept(struct socket *sock, struct socket *newsock,
 	kfree_skb(skb);
 	sk_acceptq_removed(sk);
 	newsock->state = SS_CONNECTED;
+	ax25 = sk_to_ax25(newsk);
+	ax25_dev = ax25->ax25_dev;
+	netdev_hold(ax25_dev->dev, &ax25->dev_tracker, GFP_ATOMIC);
+	ax25_dev_hold(ax25_dev);
 
 out:
 	release_sock(sk);