[PATCH v2] ax25: Fix refcount imbalance on inbound connections

[PATCH v2] ax25: Fix refcount imbalance on inbound connections

[Lars Kellogg-Stedman]

The first version of this patch was posted only to the linux-hams
mailing list. It has been difficult to get the patch reviewed, but the
patch has now been tested successfully by three people (that includes
me) who have all verified that it prevents the crashes that were
previously plaguing inbound ax.25 connections.

Related discussions:

- https://marc.info/?l=linux-hams&m=171629285223248&w=2
- https://marc.info/?l=linux-hams&m=171270115728031&w=2

>8------------------------------------------------------8<

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(), balancing the
calls to netdev_put() and ax25_dev_put() in ax25_release.

Fixes: 7d8a3a477b
Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
---
 net/ax25/af_ax25.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 8077cf2ee44..ff921272d40 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1381,6 +1381,8 @@ static int ax25_accept(struct socket *sock, struct socket *newsock,
 	DEFINE_WAIT(wait);
 	struct sock *sk;
 	int err = 0;
+	ax25_cb *ax25;
+	ax25_dev *ax25_dev;
 
 	if (sock->state != SS_UNCONNECTED)
 		return -EINVAL;
@@ -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);
-- 
2.45.1

-- 
Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
http://blog.oddbit.com/                | N1LKS

Re: [PATCH v2] ax25: Fix refcount imbalance on inbound connections

[Chris Maness]

Should I expect to this downstream in 6.9.2?

-Chris KQ6UP

On Tue, May 21, 2024 at 7:26 AM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
>
> The first version of this patch was posted only to the linux-hams
> mailing list. It has been difficult to get the patch reviewed, but the
> patch has now been tested successfully by three people (that includes
> me) who have all verified that it prevents the crashes that were
> previously plaguing inbound ax.25 connections.
>
> Related discussions:
>
> - https://marc.info/?l=linux-hams&m=171629285223248&w=2
> - https://marc.info/?l=linux-hams&m=171270115728031&w=2
>
> >8------------------------------------------------------8<
>
> 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(), balancing the
> calls to netdev_put() and ax25_dev_put() in ax25_release.
>
> Fixes: 7d8a3a477b
> Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
> ---
>  net/ax25/af_ax25.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 8077cf2ee44..ff921272d40 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -1381,6 +1381,8 @@ static int ax25_accept(struct socket *sock, struct socket *newsock,
>         DEFINE_WAIT(wait);
>         struct sock *sk;
>         int err = 0;
> +       ax25_cb *ax25;
> +       ax25_dev *ax25_dev;
>
>         if (sock->state != SS_UNCONNECTED)
>                 return -EINVAL;
> @@ -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);
> --
> 2.45.1
>
> --
> Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
> http://blog.oddbit.com/                | N1LKS
>


-- 
Thanks,
Chris Maness

RE: [PATCH v2] ax25: Fix refcount imbalance on inbound connections

[Naveen Mamindlapalli]

> -----Original Message-----
> From: Lars Kellogg-Stedman <lars@oddbit.com>
> Sent: Tuesday, May 21, 2024 7:19 PM
> To: netdev@vger.kernel.org; linux-hams@vger.kernel.org
> Subject: [PATCH v2] ax25: Fix refcount imbalance on inbound
> connections
> 
> The first version of this patch was posted only to the linux-hams mailing list. It
> has been difficult to get the patch reviewed, but the patch has now been tested
> successfully by three people (that includes
> me) who have all verified that it prevents the crashes that were previously
> plaguing inbound ax.25 connections.
> 
> Related discussions:
> 
> - https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-
> 2Dhams-26m-3D171629285223248-26w-
> 3D2&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6mQ8K9wIpqwFO8y
> jikO_w1jUOe2MzChg4Rmg&m=T0z8LSTV_ukO1CjzoYF7lKkzfKSd1iQ3b4biO_v8e
> 0R8Llg9vHP8lSQ3tGo5sXr4&s=-
> MpoDvDpxv4ixHLeXbhPn1j9UnlUz6cJ29sdjMqsA6I&e=
> - https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-
> 2Dhams-26m-3D171270115728031-26w-
> 3D2&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6mQ8K9wIpqwFO8y
> jikO_w1jUOe2MzChg4Rmg&m=T0z8LSTV_ukO1CjzoYF7lKkzfKSd1iQ3b4biO_v8e
> 0R8Llg9vHP8lSQ3tGo5sXr4&s=3ZUXfkS5ChKuooKI5BBasvYQeJMH-
> FYqbeskhJKJdOI&e=
> 
> >8------------------------------------------------------8<
> 
> 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(), balancing the calls to
> netdev_put() and ax25_dev_put() in ax25_release.
> 
> Fixes: 7d8a3a477b
> Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
> ---
>  net/ax25/af_ax25.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c index
> 8077cf2ee44..ff921272d40 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -1381,6 +1381,8 @@ static int ax25_accept(struct socket *sock, struct
> socket *newsock,
>  	DEFINE_WAIT(wait);
>  	struct sock *sk;
>  	int err = 0;
> +	ax25_cb *ax25;
> +	ax25_dev *ax25_dev;

nit: Please follow reverse Christmas tree.

> 
>  	if (sock->state != SS_UNCONNECTED)
>  		return -EINVAL;
> @@ -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);
> --
> 2.45.1
> 
> --
> Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.oddbit.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6
> mQ8K9wIpqwFO8yjikO_w1jUOe2MzChg4Rmg&m=T0z8LSTV_ukO1CjzoYF7lKkzf
> KSd1iQ3b4biO_v8e0R8Llg9vHP8lSQ3tGo5sXr4&s=8ug-soRMsh7y1vLY4tO8OSx-
> bKun8bXlZ7uAPiQnePU&e=                 | N1LKS

Re: [PATCH v2] ax25: Fix refcount imbalance on inbound connections

[Lars Kellogg-Stedman]

On Tue, May 21, 2024 at 05:21:40PM GMT, Naveen Mamindlapalli wrote:
> > socket *newsock,
> >  	DEFINE_WAIT(wait);
> >  	struct sock *sk;
> >  	int err = 0;
> > +	ax25_cb *ax25;
> > +	ax25_dev *ax25_dev;
> 
> nit: Please follow reverse Christmas tree.

That is a new phrase for me; I had to look it up. Do you mean this:

        DEFINE_WAIT(wait);
        struct sock *sk;
        int err = 0;
+	      ax25_dev *ax25_dev;
+	      ax25_cb *ax25;

Or should I apply this to the entire block of variable declarations,
like this:

        struct sk_buff *skb;
        struct sock *newsk;
+       ax25_dev *ax25_dev;
        DEFINE_WAIT(wait);
        struct sock *sk;
+       ax25_cb *ax25;
        int err = 0;

Thanks,

-- 
Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
http://blog.oddbit.com/                | N1LKS

RE: Re: [PATCH v2] ax25: Fix refcount imbalance on inbound connections

[Naveen Mamindlapalli]

> -----Original Message-----
> From: Lars Kellogg-Stedman <lars@oddbit.com>
> Sent: Tuesday, May 21, 2024 11:27 PM
> To: Naveen Mamindlapalli <naveenm@marvell.com>
> Cc: netdev@vger.kernel.org; linux-hams@vger.kernel.org
> Subject: Re: [PATCH v2] ax25: Fix refcount imbalance on inbound
> connections
> 
> On Tue, May 21, 2024 at 05:21:40PM GMT, Naveen Mamindlapalli wrote:
> > > socket *newsock,
> > >  	DEFINE_WAIT(wait);
> > >  	struct sock *sk;
> > >  	int err = 0;
> > > +	ax25_cb *ax25;
> > > +	ax25_dev *ax25_dev;
> >
> > nit: Please follow reverse Christmas tree.
> 
> That is a new phrase for me; I had to look it up. Do you mean this:
> 
>         DEFINE_WAIT(wait);
>         struct sock *sk;
>         int err = 0;
> +	      ax25_dev *ax25_dev;
> +	      ax25_cb *ax25;
> 
> Or should I apply this to the entire block of variable declarations, like this:
> 
>         struct sk_buff *skb;
>         struct sock *newsk;
> +       ax25_dev *ax25_dev;
>         DEFINE_WAIT(wait);
>         struct sock *sk;
> +       ax25_cb *ax25;
>         int err = 0;

Yes, apply reverse xmas tree order to entire block.

Thanks,
Naveen

> 
> Thanks,
> 
> --
> Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.oddbit.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TwreqwV6
> mQ8K9wIpqwFO8yjikO_w1jUOe2MzChg4Rmg&m=FtlS2pOuM2TyZSjXUe6s5L7w
> o2YtvcK9Ep3HQRqf8dMeSy9VLui3rMQDUcVMFLcK&s=0fvO6BKSQQg3niGImy4
> VLvjVZ0kOAeAjIB2WwdZNYRs&e=                 | N1LKS

[PATCH v3] ax25: Fix refcount imbalance on inbound connections

[lars]

From: Lars Kellogg-Stedman <lars@oddbit.com>

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 <lars@oddbit.com>
---
v3:
- Address naveenm's comments regarding the ordering of variable
  declarations (https://lore.kernel.org/netdev/SJ2PR18MB5635B7ADC7339BEDB79B183DA2EA2@SJ2PR18MB5635.namprd18.prod.outlook.com/)

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

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

Re: [PATCH v3] ax25: Fix refcount imbalance on inbound connections

[Jakub Kicinski]

On Tue, 21 May 2024 14:23:25 -0400 lars@oddbit.com 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)
-- 
pw-bot: cr

Re: [PATCH v3] ax25: Fix refcount imbalance on inbound connections

[Lars Kellogg-Stedman]

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:	linux-hams@vger.kernel.org
    S:	Orphan
    W:	https://linux-ax25.in-berlin.de
    F:	include/net/ax25.h
    F:	include/uapi/linux/ax25.h
    F:	net/ax25/

-- 
Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
http://blog.oddbit.com/                | N1LKS

Re: [PATCH v3] ax25: Fix refcount imbalance on inbound connections

[Jakub Kicinski]

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:	linux-hams@vger.kernel.org
>     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