[PATCH net] ax25: Fix refcount leak issues of ax25_dev

[PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Duoming Zhou]

There are two scenarios that might cause refcount leak
issues of ax25_dev.

Scenario one:

The refcount of ax25_dev potentially increase more
than once in ax25_addr_ax25dev(), which will cause
memory leak.

In order to fix the above issue, only increase the
refcount of ax25_dev once, when the res is not null.

Scenario two:

The original code sets the refcount of ax25_dev to 1
in the initial stage and then increase the refcount
when the ax25_dev is added to the ax25_dev_list. As a
result, the refcount of ax25_dev is 2. But when the
device is shutting down. The ax25_dev_device_down()
drops the refcount once or twice depending on if we
goto unlock_put or not, which will cause memory leak.

In order to mitigate the above issues, only increase
the refcount of ax25_dev when the ax25_dev is added
to the ax25_dev_list and decrease the refcount of
ax25_dev after it is removed from the ax25_dev_list.

What's more, the ax25_dev should not be deallocated
directly by kfree() in ax25_dev_free(), replace it
with ax25_dev_put() instead.

Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
Reported by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 net/ax25/ax25_dev.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 282ec581c07..0e6dd98d3fa 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -37,8 +37,9 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
 	for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
 		if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
 			res = ax25_dev;
-			ax25_dev_hold(ax25_dev);
 		}
+	if (res)
+		ax25_dev_hold(res);
 	spin_unlock_bh(&ax25_dev_lock);
 
 	return res;
@@ -58,7 +59,6 @@ void ax25_dev_device_up(struct net_device *dev)
 		return;
 	}
 
-	refcount_set(&ax25_dev->refcount, 1);
 	dev->ax25_ptr     = ax25_dev;
 	ax25_dev->dev     = dev;
 	netdev_hold(dev, &ax25_dev->dev_tracker, GFP_KERNEL);
@@ -88,7 +88,7 @@ void ax25_dev_device_up(struct net_device *dev)
 	ax25_dev->next = ax25_dev_list;
 	ax25_dev_list  = ax25_dev;
 	spin_unlock_bh(&ax25_dev_lock);
-	ax25_dev_hold(ax25_dev);
+	refcount_set(&ax25_dev->refcount, 1);
 
 	ax25_register_dev_sysctl(ax25_dev);
 }
@@ -135,7 +135,6 @@ void ax25_dev_device_down(struct net_device *dev)
 
 unlock_put:
 	spin_unlock_bh(&ax25_dev_lock);
-	ax25_dev_put(ax25_dev);
 	dev->ax25_ptr = NULL;
 	netdev_put(dev, &ax25_dev->dev_tracker);
 	ax25_dev_put(ax25_dev);
@@ -208,7 +207,7 @@ void __exit ax25_dev_free(void)
 		s        = ax25_dev;
 		netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker);
 		ax25_dev = ax25_dev->next;
-		kfree(s);
+		ax25_dev_put(s);
 	}
 	ax25_dev_list = NULL;
 	spin_unlock_bh(&ax25_dev_lock);
-- 
2.17.1

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Markus Elfring]

…
> In order to mitigate the above issues, only increase
> the refcount of ax25_dev when the ax25_dev is added
> to the ax25_dev_list and decrease the refcount of
> ax25_dev after it is removed from the ax25_dev_list.
…

* I suggest to use more than 53 characters in lines of such a change description.

* Can it be nicer to mention also the term “reference counting” for
  an improved commit message?

Regards,
Markus

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

I'm always happy to take credit for stuff but the Reported by should go
to Lars and Miroslav.

Reported-by: Lars Kellogg-Stedman <lars@oddbit.com>
Reported-by: Miroslav Skoric <skoric@uns.ac.rs>

Lars, could you test this please and let us know if it helps?

regards,
dan carpenter

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[duoming]

On Wed, 1 May 2024 20:43:37 +0300 Dan Carpenter wrote:
> I'm always happy to take credit for stuff but the Reported by should go
> to Lars and Miroslav.
> 
> Reported-by: Lars Kellogg-Stedman <lars@oddbit.com>
> Reported-by: Miroslav Skoric <skoric@uns.ac.rs>

This patch is not related with the problem raised by Lars Kellogg-Stedman
and Miroslav Skoric, it only solves the reference counting leak issues of
ax25_dev in ax25_addr_ax25dev() and ax25_dev_device_down(). So I think
there is no need to change the "Reported by" label.

Best regards,
Duoming Zhou

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

On Thu, May 02, 2024 at 12:35:44PM +0800, duoming@zju.edu.cn wrote:
> On Wed, 1 May 2024 20:43:37 +0300 Dan Carpenter wrote:
> > I'm always happy to take credit for stuff but the Reported by should go
> > to Lars and Miroslav.
> > 
> > Reported-by: Lars Kellogg-Stedman <lars@oddbit.com>
> > Reported-by: Miroslav Skoric <skoric@uns.ac.rs>
> 
> This patch is not related with the problem raised by Lars Kellogg-Stedman
> and Miroslav Skoric, it only solves the reference counting leak issues of
> ax25_dev in ax25_addr_ax25dev() and ax25_dev_device_down(). So I think
> there is no need to change the "Reported by" label.
> 

Ah...  I was really hoping it was related to the other bugs.

Okay, what about we separate this into different patches one for each
bug?  The changes to ax25_addr_ax25dev() and ax25_dev_free() are
obvious and could go in as-is but as two separate patches.

The changes to ax25_dev_device_up/down() are more subtle.

The ax25_dev_list stuff is frustrating.  It would be so much easier if
it were a normal list and you could just do:

        /*
         *      Remove any packet forwarding that points to this device.
         */
        list_for_each_entry(s, ax25_dev_list, list) {
                if (s->forward == dev)
                        s->forward = NULL;
        }

        list_for_each_entry(s, ax25_dev_list, list) {
                if (s == ax25_dev) {
                        list_del(s);
                        free_net = true;
                        break;
                }
        }

        spin_unlock_bh(&ax25_dev_lock);
        dev->ax25_ptr = NULL;
        if (free_net)
                netdev_put(dev, &ax25_dev->dev_tracker);
        ax25_dev_put(ax25_dev);
}

Why do we call netdev_put() on that one path?  Btw, here is an untested
conversion to lists...

regards,
dan carpenter

diff --git a/include/net/ax25.h b/include/net/ax25.h
index 0d939e5aee4e..c2a85fd3f5ea 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -216,7 +216,7 @@ typedef struct {
 struct ctl_table;
 
 typedef struct ax25_dev {
-	struct ax25_dev		*next;
+	struct list_head	list;
 
 	struct net_device	*dev;
 	netdevice_tracker	dev_tracker;
@@ -330,7 +330,6 @@ int ax25_addr_size(const ax25_digi *);
 void ax25_digi_invert(const ax25_digi *, ax25_digi *);
 
 /* ax25_dev.c */
-extern ax25_dev *ax25_dev_list;
 extern spinlock_t ax25_dev_lock;
 
 #if IS_ENABLED(CONFIG_AX25)
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 282ec581c072..b632af38f1e1 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -22,11 +22,12 @@
 #include <net/sock.h>
 #include <linux/uaccess.h>
 #include <linux/fcntl.h>
+#include <linux/list.h>
 #include <linux/mm.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
 
-ax25_dev *ax25_dev_list;
+static struct list_head ax25_dev_list;
 DEFINE_SPINLOCK(ax25_dev_lock);
 
 ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
@@ -34,11 +35,12 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
 	ax25_dev *ax25_dev, *res = NULL;
 
 	spin_lock_bh(&ax25_dev_lock);
-	for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next)
+	list_for_each_entry(ax25_dev, &ax25_dev_list, list) {
 		if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
 			res = ax25_dev;
 			ax25_dev_hold(ax25_dev);
 		}
+	}
 	spin_unlock_bh(&ax25_dev_lock);
 
 	return res;
@@ -52,6 +54,10 @@ void ax25_dev_device_up(struct net_device *dev)
 {
 	ax25_dev *ax25_dev;
 
+	// FIXME: do call this in probe or something
+	if (!ax25_dev_list.next)
+		INIT_LIST_HEAD(&ax25_dev_list);
+
 	ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_KERNEL);
 	if (!ax25_dev) {
 		printk(KERN_ERR "AX.25: ax25_dev_device_up - out of memory\n");
@@ -85,8 +91,7 @@ void ax25_dev_device_up(struct net_device *dev)
 #endif
 
 	spin_lock_bh(&ax25_dev_lock);
-	ax25_dev->next = ax25_dev_list;
-	ax25_dev_list  = ax25_dev;
+	list_add(&ax25_dev->list, &ax25_dev_list);
 	spin_unlock_bh(&ax25_dev_lock);
 	ax25_dev_hold(ax25_dev);
 
@@ -111,23 +116,18 @@ void ax25_dev_device_down(struct net_device *dev)
 	/*
 	 *	Remove any packet forwarding that points to this device.
 	 */
-	for (s = ax25_dev_list; s != NULL; s = s->next)
+	list_for_each_entry(s, &ax25_dev_list, list) {
 		if (s->forward == dev)
 			s->forward = NULL;
-
-	if ((s = ax25_dev_list) == ax25_dev) {
-		ax25_dev_list = s->next;
-		goto unlock_put;
 	}
 
-	while (s != NULL && s->next != NULL) {
-		if (s->next == ax25_dev) {
-			s->next = ax25_dev->next;
+	list_for_each_entry(s, &ax25_dev_list, list) {
+		if (s == ax25_dev) {
+			list_del(&s->list);
 			goto unlock_put;
 		}
-
-		s = s->next;
 	}
+
 	spin_unlock_bh(&ax25_dev_lock);
 	dev->ax25_ptr = NULL;
 	ax25_dev_put(ax25_dev);
@@ -200,16 +200,13 @@ struct net_device *ax25_fwd_dev(struct net_device *dev)
  */
 void __exit ax25_dev_free(void)
 {
-	ax25_dev *s, *ax25_dev;
+	ax25_dev *s, *n;
 
 	spin_lock_bh(&ax25_dev_lock);
-	ax25_dev = ax25_dev_list;
-	while (ax25_dev != NULL) {
-		s        = ax25_dev;
-		netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker);
-		ax25_dev = ax25_dev->next;
+	list_for_each_entry_safe(s, n, &ax25_dev_list, list) {
+		netdev_put(s->dev, &s->dev_tracker);
+		list_del(&s->list);
 		kfree(s);
 	}
-	ax25_dev_list = NULL;
 	spin_unlock_bh(&ax25_dev_lock);
 }

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Paolo Abeni]

On Thu, 2024-05-02 at 10:56 +0300, Dan Carpenter wrote:
> On Thu, May 02, 2024 at 12:35:44PM +0800, duoming@zju.edu.cn wrote:
> > On Wed, 1 May 2024 20:43:37 +0300 Dan Carpenter wrote:
> > > I'm always happy to take credit for stuff but the Reported by should go
> > > to Lars and Miroslav.
> > > 
> > > Reported-by: Lars Kellogg-Stedman <lars@oddbit.com>
> > > Reported-by: Miroslav Skoric <skoric@uns.ac.rs>
> > 
> > This patch is not related with the problem raised by Lars Kellogg-Stedman
> > and Miroslav Skoric, it only solves the reference counting leak issues of
> > ax25_dev in ax25_addr_ax25dev() and ax25_dev_device_down(). So I think
> > there is no need to change the "Reported by" label.
> > 
> 
> Ah...  I was really hoping it was related to the other bugs.
> 
> Okay, what about we separate this into different patches one for each
> bug?  The changes to ax25_addr_ax25dev() and ax25_dev_free() are
> obvious and could go in as-is but as two separate patches.

I agree it would be better to split this up, the changelog itself hints
at 2 separated issues and fixes.

Thanks,

Paolo

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Lars Kellogg-Stedman]

On Wed, May 01, 2024 at 02:02:18PM +0800, Duoming Zhou wrote:
> There are two scenarios that might cause refcount leak
> issues of ax25_dev.

This patch doesn't address the refcount leaks I reported earlier and
resolved in the patch I posted [1] last week.

Assume we have the following two interfaces configured on a system:

    $ cat /etc/ax25/axports
    udp0 test0-0 9600 255 2 axudp0
    udp1 test0-1 9600 255 2 axudp1

And we have ax25d listening on both interfaces:

    [udp0]
    default  * * * * * *  - root  /usr/sbin/axwrapper axwrapper -- /bin/sh sh /etc/ax25/example-output.sh
    [udp1]
    default  * * * * * *  - root  /usr/sbin/axwrapper axwrapper -- /bin/sh sh /etc/ax25/example-output.sh

Using the 'ax-devs' and 'ax-sockets' gdb commands shown at the end of
this message, we start with:

    (gdb) ax-devs
    ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
    ax0 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
    (gdb) ax-sockets
    0xffff8881002b6800 if:ax1 state:0 refcnt:2 dev_tracker:0xffff888100ded200
    0xffff888101ac4e00 if:ax0 state:0 refcnt:2 dev_tracker:0xffff888100dec4c0

We initiate a connection from ax0 to ax1:

    call -r udp0 test0-1

When we first enter ax25_rcv, we have:

    (gdb) ax-devs
    ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
    ax0 ax_refcnt:3 dev_refcnt:10 dev_untracked:1 dev_notrack:1
    (gdb) ax-sockets
    0xffff888101ac8000 if:ax0 state:1 refcnt:2 dev_tracker:0xffff888100dedb80
    0xffff8881002b6800 if:ax1 state:0 refcnt:2 dev_tracker:0xffff888100ded200
    0xffff888101ac4e00 if:ax0 state:0 refcnt:2 dev_tracker:0xffff888100dec4c0

After we reach line 413 (in net/ax25/ax25_in.c) and add a new control
block:

    ax25_cb_add(ax25)

We have:

    (gdb) ax-devs
    ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
    ax0 ax_refcnt:3 dev_refcnt:10 dev_untracked:1 dev_notrack:1
    (gdb) ax-sockets
    0xffff88810245ac00 if:ax1 state:3 refcnt:2 dev_tracker:0x0 <fixed_percpu_data>
    0xffff88810245ba00 if:ax0 state:1 refcnt:2 dev_tracker:0xffff88810136c800
    0xffff888100c79e00 if:ax1 state:0 refcnt:2 dev_tracker:0xffff88810136c6e0
    0xffff8881018e9800 if:ax0 state:0 refcnt:2 dev_tracker:0xffff88810170c860

Note that (a) ax25->dev_tracker is NULL, and (b) we have incremeted the
refcount on ax0 (the source interface), but not on ax1 (the destination
interface). When we call ax25_release for this control block, we get to:

    netdev_put(ax25_dev->dev, &ax25->dev_tracker);
    ax25_dev_put(ax25_dev);

With:

    (gdb) ax-devs
    ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
    ax0 ax_refcnt:3 dev_refcnt:10 dev_untracked:1 dev_notrack:1

After the calls to netdev_put() and ax25_dev_put(), we have:

    (gdb) ax-devs
    ax1 ax_refcnt:1 dev_refcnt:8 dev_untracked:-1073741824 dev_notrack:1
    ax0 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1

You can see that (a) ax25_dev->dev->refcnt_tracker->untracked is now
invalid, and ax25_dev->dev->dev_refcnt is in trouble: it decrements by
one for each closed connection, even though it was never incremented
when we accepted the connection. The underflow in
...refcnt_tracker->untracked yields the traceback with:

    refcount_t: decrement hit 0; leaking memory.

Additional connections will eventually trigger more problems; we will
ultimately underflow ax25_dev->dev->dev_refcnt, but we may also run into
memory corruption because of the invalid tracker data, resulting in:

    BUG: unable to handle page fault for address: 00000010000003b0

The patch I submitted last week resolves all of the above issues and has
no refcount leaks for this particular code path. In order to avoid the
refcount leaks, those _put() calls in ax25_release need to be balanced
by _hold() calls when accepting a new connection (or we need to wrap
them in a conditional so that they're not called when ax25->dev_tracker
is NULL).

GDB commands:

    define ax-devs
      set $x = ax25_dev_list
      while ($x != 0)
        printf "%s ax_refcnt:%d dev_refcnt:%d dev_untracked:%d dev_notrack:%d\n", $x->dev->name, \
          $x->refcount->refs->counter, \
          $x->dev->dev_refcnt->refs->counter, \
          $x->dev->refcnt_tracker->untracked->refs->counter, \
          $x->dev->refcnt_tracker->no_tracker->refs->counter
        set $x = $x->next
      end
    end

    define ax-sockets
      set $x = ax25_list->first
      while ($x != 0)
        set $cb = (ax25_cb *)($x)

        printf "%s if:%s state:%d refcnt:%d dev_tracker:%s\n", \
          $_as_string($cb), \
          $cb->ax25_dev->dev->name, \
          $cb->state, \
          $cb->refcount->refs->counter, \
          $_as_string($cb->dev_tracker)
        set $x = $x->next
      end
    end

[1]: https://marc.info/?l=linux-hams&m=171447153903965&w=2

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

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

Could you test this diff?

regards,
dan carpenter

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 558e158c98d0..a7f96a4ceff4 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1129,8 +1129,10 @@ static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	/*
 	 * User already set interface with SO_BINDTODEVICE
 	 */
-	if (ax25->ax25_dev != NULL)
+	if (ax25->ax25_dev != NULL) {
+		ax25_dev_hold(ax25->ax25_dev);
 		goto done;
+	}
 
 	if (addr_len > sizeof(struct sockaddr_ax25) && addr->fsa_ax25.sax25_ndigis == 1) {
 		if (ax25cmp(&addr->fsa_digipeater[0], &null_ax25_address) != 0 &&

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Lars Kellogg-Stedman]

On Fri, May 03, 2024 at 11:36:37PM +0300, Dan Carpenter wrote:
> Could you test this diff?

With that diff applied, there is no kernel panic, but I see the same
refcount errors that I saw before the latest series of patches from
Duoming:

  refcount_t: decrement hit 0; leaking memory.
  refcount_t: underflow; use-after-free.

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

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

On Fri, May 03, 2024 at 07:40:32PM -0400, Lars Kellogg-Stedman wrote:
> On Fri, May 03, 2024 at 11:36:37PM +0300, Dan Carpenter wrote:
> > Could you test this diff?
> 
> With that diff applied, there is no kernel panic, but I see the same
> refcount errors that I saw before the latest series of patches from
> Duoming:

Wait, which panic is this?  The NULL dereference introduce by the
"ax25_dev" vs "res" typo?

> 
>   refcount_t: decrement hit 0; leaking memory.
>   refcount_t: underflow; use-after-free.

Hm...  Is there a missing netdev_hold() in ax25_bind() on the
"User already set interface with SO_BINDTODEVICE" path?  That would
fit with the commit 9fd75b66b8f6 ("ax25: Fix refcount leaks caused by
ax25_cb_del()") which introduced the bug.

I'm not really sure I understand how netdev_hold() works.

(My patch here is correct, but apparently that's not the bug).

regards,
dan carpenter

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Lars Kellogg-Stedman]

On Sat, May 04, 2024 at 03:16:55PM +0300, Dan Carpenter wrote:
> Wait, which panic is this?  The NULL dereference introduce by the
> "ax25_dev" vs "res" typo?

Right, that one. Your diff was on top of Duoming's patches, which had
earlier introduced that kernel panic. Just confirming that things are
working (for some value of "working") with your latest change.

> >   refcount_t: decrement hit 0; leaking memory.
> >   refcount_t: underflow; use-after-free.
> 
> Hm...  Is there a missing netdev_hold() in ax25_bind() on the
> "User already set interface with SO_BINDTODEVICE" path?

There's a missing netdev_hold() in the path for *inbound* packets
(ax25_rcv -> ax25_accept).

I added some tracepoints [1] so that we can see calls to
netdev_{put,hold} in a function graph. Without my patches, the graph for
an incoming connections looks like [2]:

    # tracer: function_graph
    #
    # CPU  TASK/PID         DURATION                  FUNCTION CALLS
    # |     |    |           |   |                     |   |   |   |
    ------------------------------------------
    0)    <idle>-0    =>   kworker-29  
    ------------------------------------------

    0)   kworker-29   |               |  ax25_kiss_rcv() {
    0)   kworker-29   |               |    ax25_rcv.isra.0() {
    0)   kworker-29   |   0.168 us    |      ax25_addr_parse();
    0)   kworker-29   |   0.094 us    |      ax25_addr_size();
    0)   kworker-29   |   0.136 us    |      ax25cmp();
    0)   kworker-29   |   0.137 us    |      ax25_digi_invert();
    0)   kworker-29   |               |      ax25_find_cb() {
    0)   kworker-29   |   0.223 us    |        ax25cmp();
    0)   kworker-29   |   0.194 us    |        ax25cmp();
    0)   kworker-29   |   0.087 us    |        ax25cmp();
    0)   kworker-29   |   0.975 us    |      }
    0)   kworker-29   |               |      ax25_find_listener() {
    0)   kworker-29   |   0.092 us    |        ax25cmp();
    0)   kworker-29   |   0.084 us    |        ax25cmp();
    0)   kworker-29   |   0.526 us    |      }
    0)   kworker-29   |               |      ax25_make_new() {
    0)   kworker-29   |               |        ax25_create_cb() {
    0)   kworker-29   |   0.117 us    |          ax25_setup_timers();
    0)   kworker-29   |   0.478 us    |        }
    0)   kworker-29   |   1.813 us    |      }
    0)   kworker-29   |               |      ax25_send_control() {
    0)   kworker-29   |               |        ax25_transmit_buffer() {
    0)   kworker-29   |   0.086 us    |          ax25_addr_size();
    0)   kworker-29   |   0.094 us    |          ax25_addr_build();
    0)   kworker-29   |   0.081 us    |          ax25_fwd_dev();
    0)   kworker-29   |   4.854 us    |        }
    0)   kworker-29   |   5.604 us    |      }
    0)   kworker-29   |   0.108 us    |      ax25_cb_add();
    0)   kworker-29   |   0.410 us    |      ax25_start_heartbeat();
    0)   kworker-29   |   0.218 us    |      ax25_start_t3timer();
    0)   kworker-29   |   0.099 us    |      ax25_start_idletimer();
    0)   kworker-29   | + 14.957 us   |    }
    0)   kworker-29   | + 16.116 us   |  }
    ------------------------------------------
    1)   ax25ipd-63   =>    ax25d-77   
    ------------------------------------------

    1)    ax25d-77    |   1.580 us    |  ax25_accept();
    1)    ax25d-77    |   0.211 us    |  ax25_getname();
    1)    ax25d-77    |   0.275 us    |  ax25_getname();
    ------------------------------------------
    0)   kworker-29   =>   axwrapp-83  
    ------------------------------------------

    0)   axwrapp-83   |               |  ax25_sendmsg() {
    0)   axwrapp-83   |               |    ax25_output() {
    0)   axwrapp-83   |               |      ax25_kick.part.0() {
    0)   axwrapp-83   |               |        ax25_send_iframe() {
    0)   axwrapp-83   |   0.679 us    |          ax25_start_idletimer();
    0)   axwrapp-83   |               |          ax25_transmit_buffer() {
    0)   axwrapp-83   |   0.093 us    |            ax25_addr_size();
    0)   axwrapp-83   |   0.102 us    |            ax25_addr_build();
    0)   axwrapp-83   |   0.145 us    |            ax25_fwd_dev();
    0)   axwrapp-83   |   9.311 us    |          }
    0)   axwrapp-83   | + 10.464 us   |        }
    0)   axwrapp-83   |   0.096 us    |        ax25_t1timer_running();
    0)   axwrapp-83   |   0.332 us    |        ax25_stop_t3timer();
    0)   axwrapp-83   |   0.093 us    |        ax25_calculate_t1();
    0)   axwrapp-83   |   0.439 us    |        ax25_start_t1timer();
    0)   axwrapp-83   | + 18.839 us   |      }
    0)   axwrapp-83   | + 19.479 us   |    }
    0)   axwrapp-83   | + 22.647 us   |  }
    ------------------------------------------
    0)   ax25ipd-63   =>   axwrapp-83  
    ------------------------------------------

    0)   axwrapp-83   |               |  ax25_sendmsg() {
    0)   axwrapp-83   |               |    ax25_output() {
    0)   axwrapp-83   |               |      ax25_kick.part.0() {
    0)   axwrapp-83   |               |        ax25_send_iframe() {
    0)   axwrapp-83   |   0.339 us    |          ax25_start_idletimer();
    0)   axwrapp-83   |               |          ax25_transmit_buffer() {
    0)   axwrapp-83   |   0.092 us    |            ax25_addr_size();
    0)   axwrapp-83   |   0.097 us    |            ax25_addr_build();
    0)   axwrapp-83   |   0.136 us    |            ax25_fwd_dev();
    0)   axwrapp-83   |   9.116 us    |          }
    0)   axwrapp-83   |   9.908 us    |        }
    0)   axwrapp-83   |   0.091 us    |        ax25_t1timer_running();
    0)   axwrapp-83   | + 10.810 us   |      }
    0)   axwrapp-83   | + 11.168 us   |    }
    0)   axwrapp-83   | + 14.362 us   |  }
    ------------------------------------------
    0)   axwrapp-83   =>   kworker-10  
    ------------------------------------------

    0)   kworker-10   |               |  ax25_kiss_rcv() {
    0)   kworker-10   |               |    ax25_rcv.isra.0() {
    0)   kworker-10   |   0.116 us    |      ax25_addr_parse();
    0)   kworker-10   |   0.090 us    |      ax25_addr_size();
    0)   kworker-10   |   0.144 us    |      ax25cmp();
    0)   kworker-10   |   0.091 us    |      ax25_digi_invert();
    0)   kworker-10   |               |      ax25_find_cb() {
    0)   kworker-10   |   0.091 us    |        ax25cmp();
    0)   kworker-10   |   0.087 us    |        ax25cmp();
    0)   kworker-10   |   0.550 us    |      }
    0)   kworker-10   |               |      ax25_std_frame_in() {
    0)   kworker-10   |   0.105 us    |        ax25_decode();
    0)   kworker-10   |   0.117 us    |        ax25_validate_nr();
    0)   kworker-10   |               |        ax25_check_iframes_acked() {
    0)   kworker-10   |   0.670 us    |          ax25_frames_acked();
    0)   kworker-10   |               |          ax25_calculate_rtt() {
    0)   kworker-10   |   0.086 us    |            ax25_t1timer_running();
    0)   kworker-10   |   0.085 us    |            ax25_display_timer();
    0)   kworker-10   |   0.465 us    |          }
    0)   kworker-10   |   0.185 us    |          ax25_stop_t1timer();
    0)   kworker-10   |   0.358 us    |          ax25_start_t3timer();
    0)   kworker-10   |   2.186 us    |        }
    0)   kworker-10   |               |        ax25_kick() {
    0)   kworker-10   |   0.093 us    |          ax25_kick.part.0();
    0)   kworker-10   |   0.277 us    |        }
    0)   kworker-10   |   3.320 us    |      }
    0)   kworker-10   |   5.927 us    |    }
    0)   kworker-10   |   6.563 us    |  }
    ------------------------------------------
    0)   kworker-10   =>   axwrapp-83  
    ------------------------------------------

    0)   axwrapp-83   |               |  ax25_release() {
    0)   axwrapp-83   |   0.158 us    |    ax25_clear_queues();
    0)   axwrapp-83   |               |    ax25_send_control() {
    0)   axwrapp-83   |               |      ax25_transmit_buffer() {
    0)   axwrapp-83   |   0.086 us    |        ax25_addr_size();
    0)   axwrapp-83   |   0.086 us    |        ax25_addr_build();
    0)   axwrapp-83   |   0.085 us    |        ax25_fwd_dev();
    0)   axwrapp-83   |   3.972 us    |      }
    0)   axwrapp-83   |   4.356 us    |    }
    0)   axwrapp-83   |   0.142 us    |    ax25_stop_t2timer();
    0)   axwrapp-83   |   0.145 us    |    ax25_stop_t3timer();
    0)   axwrapp-83   |   0.094 us    |    ax25_stop_idletimer();
    0)   axwrapp-83   |   0.093 us    |    ax25_calculate_t1();
    0)   axwrapp-83   |   0.227 us    |    ax25_start_t1timer();
    0)   axwrapp-83   |               |  /* netdev_put: dev=ax0 */
    ------------------------------------------
    1)    ax25d-77    =>   kworker-10  
    ------------------------------------------

    1)   kworker-10   |               |  ax25_kiss_rcv() {
    1)   kworker-10   |               |    ax25_rcv.isra.0() {
    1)   kworker-10   |   0.188 us    |      ax25_addr_parse();
    1)   kworker-10   |   0.089 us    |      ax25_addr_size();
    1)   kworker-10   |   0.177 us    |      ax25cmp();
    1)   kworker-10   |   0.101 us    |      ax25_digi_invert();
    1)   kworker-10   |               |      ax25_find_cb() {
    1)   kworker-10   |   0.092 us    |        ax25cmp();
    1)   kworker-10   |   0.085 us    |        ax25cmp();
    1)   kworker-10   |   0.731 us    |      }
    1)   kworker-10   |               |      ax25_std_frame_in() {
    1)   kworker-10   |   0.098 us    |        ax25_decode();
    1)   kworker-10   |               |        ax25_disconnect() {
    1)   kworker-10   |   0.311 us    |          ax25_stop_t1timer();
    1)   kworker-10   |   0.093 us    |          ax25_stop_t2timer();
    1)   kworker-10   |   0.093 us    |          ax25_stop_t3timer();
    1)   kworker-10   |   0.086 us    |          ax25_stop_idletimer();
    1)   kworker-10   |   0.415 us    |          ax25_link_failed();
    1)   kworker-10   |   1.778 us    |        }
    1)   kworker-10   |   0.090 us    |        ax25_kick();
    1)   kworker-10   |   2.523 us    |      }
    1)   kworker-10   | + 10.078 us   |    }
    1)   kworker-10   | + 12.054 us   |  }
    0)   axwrapp-83   | * 13227.13 us |  }
    ------------------------------------------
    0)   axwrapp-83   =>    <idle>-0   
    ------------------------------------------

    0)    <idle>-0    |               |  ax25_heartbeat_expiry() {
    0)    <idle>-0    |               |    ax25_std_heartbeat_expiry() {
    0)    <idle>-0    |               |      ax25_destroy_socket() {
    0)    <idle>-0    |   0.249 us    |        ax25_cb_del();
    0)    <idle>-0    |   0.107 us    |        ax25_stop_heartbeat();
    0)    <idle>-0    |   0.140 us    |        ax25_stop_t1timer();
    0)    <idle>-0    |   0.090 us    |        ax25_stop_t2timer();
    0)    <idle>-0    |   0.146 us    |        ax25_stop_t3timer();
    0)    <idle>-0    |   0.089 us    |        ax25_stop_idletimer();
    0)    <idle>-0    |   0.740 us    |        ax25_clear_queues();
    0)    <idle>-0    |   2.703 us    |      }
    0)    <idle>-0    |   0.901 us    |      ax25_free_sock();
    0)    <idle>-0    |   5.219 us    |    }
    0)    <idle>-0    |   7.220 us    |  }

Note the call to netdev_put() in ax25_release(), and note that there is
never a corresponding call to netdev_hold(), which is why we end up with
a refcount problem.

My original patch corrected this by adding the call to netdev_hold()
right next to the ax25_cb_add() in ax25_rcv(), which solves this
problem. If it seems weird to have this login in ax25_rcv, we could move
it to ax25_accept, right around line 1430 [3]; that would look
something like:

    diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
    index 8df2b00526e..e1ce1eeed7d 100644
    --- a/net/ax25/af_ax25.c
    +++ b/net/ax25/af_ax25.c
    @@ -1383,6 +1383,8 @@ static int ax25_accept(struct socket *sock, struct socket *newsock, int flags,
            DEFINE_WAIT(wait);
            struct sock *sk;
            int err = 0;
    +       ax25_cb *ax25;
    +       ax25_dev *ax25_dev;

            if (sock->state != SS_UNCONNECTED)
                    return -EINVAL;
    @@ -1436,6 +1438,10 @@ static int ax25_accept(struct socket *sock, struct socket *newsock, int flags,
            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);

This has the advantage that now ax25_accept() mirrors the behavior of
ax25_bind() in terms of declaring a reference on the ax25 device, which
makes a certain amount of sense.

This seems to work out just as well as the previous patch; both
eliminiate the refcount imbalance. With the above diff in place, the
function graph looks like [4], with the call to netdev_hold() in
ax25_accept:

    # tracer: function_graph
    #
    # CPU  TASK/PID         DURATION                  FUNCTION CALLS
    # |     |    |           |   |                     |   |   |   |
    ------------------------------------------
    1)    <idle>-0    =>   kworker-29  
    ------------------------------------------

    1)   kworker-29   |               |  ax25_kiss_rcv() {
    1)   kworker-29   |               |    ax25_rcv.isra.0() {
    1)   kworker-29   |   0.152 us    |      ax25_addr_parse();
    1)   kworker-29   |   0.111 us    |      ax25_addr_size();
    1)   kworker-29   |   0.149 us    |      ax25cmp();
    1)   kworker-29   |   0.139 us    |      ax25_digi_invert();
    1)   kworker-29   |               |      ax25_find_cb() {
    1)   kworker-29   |   0.229 us    |        ax25cmp();
    1)   kworker-29   |   0.243 us    |        ax25cmp();
    1)   kworker-29   |   0.088 us    |        ax25cmp();
    1)   kworker-29   |   1.076 us    |      }
    1)   kworker-29   |               |      ax25_find_listener() {
    1)   kworker-29   |   0.092 us    |        ax25cmp();
    1)   kworker-29   |   0.085 us    |        ax25cmp();
    1)   kworker-29   |   0.632 us    |      }
    1)   kworker-29   |               |      ax25_make_new() {
    1)   kworker-29   |               |        ax25_create_cb() {
    1)   kworker-29   |   0.132 us    |          ax25_setup_timers();
    1)   kworker-29   |   0.547 us    |        }
    1)   kworker-29   |   1.965 us    |      }
    1)   kworker-29   |               |      ax25_send_control() {
    1)   kworker-29   |               |        ax25_transmit_buffer() {
    1)   kworker-29   |   0.090 us    |          ax25_addr_size();
    1)   kworker-29   |   0.089 us    |          ax25_addr_build();
    1)   kworker-29   |   0.084 us    |          ax25_fwd_dev();
    1)   kworker-29   |   5.682 us    |        }
    1)   kworker-29   |   6.037 us    |      }
    1)   kworker-29   |   0.100 us    |      ax25_cb_add();
    1)   kworker-29   |   0.449 us    |      ax25_start_heartbeat();
    1)   kworker-29   |   0.200 us    |      ax25_start_t3timer();
    1)   kworker-29   |   0.104 us    |      ax25_start_idletimer();
    1)   kworker-29   | + 16.757 us   |    }
    1)   kworker-29   | + 18.240 us   |  }
    ------------------------------------------
    0)   ax25ipd-63   =>    ax25d-77   
    ------------------------------------------

    0)    ax25d-77    |               |  ax25_accept() {
    0)    ax25d-77    |               |  /* netdev_hold: dev=ax0 */
    0)    ax25d-77    |   2.067 us    |  }
    0)    ax25d-77    |   0.140 us    |  ax25_getname();
    0)    ax25d-77    |   0.189 us    |  ax25_getname();
    ------------------------------------------
    1)   kworker-29   =>   axwrapp-82  
    ------------------------------------------

    1)   axwrapp-82   |               |  ax25_sendmsg() {
    1)   axwrapp-82   |               |    ax25_output() {
    1)   axwrapp-82   |               |      ax25_kick.part.0() {
    1)   axwrapp-82   |               |        ax25_send_iframe() {
    1)   axwrapp-82   |   0.343 us    |          ax25_start_idletimer();
    1)   axwrapp-82   |               |          ax25_transmit_buffer() {
    1)   axwrapp-82   |   0.119 us    |            ax25_addr_size();
    1)   axwrapp-82   |   0.118 us    |            ax25_addr_build();
    1)   axwrapp-82   |   0.156 us    |            ax25_fwd_dev();
    1)   axwrapp-82   |   9.965 us    |          }
    1)   axwrapp-82   | + 10.844 us   |        }
    1)   axwrapp-82   |   0.107 us    |        ax25_t1timer_running();
    1)   axwrapp-82   |   0.345 us    |        ax25_stop_t3timer();
    1)   axwrapp-82   |   0.112 us    |        ax25_calculate_t1();
    1)   axwrapp-82   |   0.499 us    |        ax25_start_t1timer();
    1)   axwrapp-82   | + 21.187 us   |      }
    1)   axwrapp-82   | + 21.620 us   |    }
    1)   axwrapp-82   | + 25.665 us   |  }
    ------------------------------------------
    1)   ax25ipd-63   =>   axwrapp-82  
    ------------------------------------------

    1)   axwrapp-82   |               |  ax25_sendmsg() {
    1)   axwrapp-82   |               |    ax25_output() {
    1)   axwrapp-82   |               |      ax25_kick.part.0() {
    1)   axwrapp-82   |               |        ax25_send_iframe() {
    1)   axwrapp-82   |   1.027 us    |          ax25_start_idletimer();
    1)   axwrapp-82   |               |          ax25_transmit_buffer() {
    1)   axwrapp-82   |   0.505 us    |            ax25_addr_size();
    1)   axwrapp-82   |   0.522 us    |            ax25_addr_build();
    1)   axwrapp-82   |   0.550 us    |            ax25_fwd_dev();
    1)   axwrapp-82   | + 32.110 us   |          }
    1)   axwrapp-82   | + 35.118 us   |        }
    1)   axwrapp-82   |   0.526 us    |        ax25_t1timer_running();
    1)   axwrapp-82   | + 38.770 us   |      }
    1)   axwrapp-82   | + 40.359 us   |    }
    1)   axwrapp-82   | + 50.019 us   |  }
    ------------------------------------------
    0)    ax25d-77    =>   kworker-10  
    ------------------------------------------

    0)   kworker-10   |               |  ax25_kiss_rcv() {
    0)   kworker-10   |               |    ax25_rcv.isra.0() {
    0)   kworker-10   |   0.632 us    |      ax25_addr_parse();
    0)   kworker-10   |   0.467 us    |      ax25_addr_size();
    0)   kworker-10   |   0.600 us    |      ax25cmp();
    0)   kworker-10   |   0.506 us    |      ax25_digi_invert();
    0)   kworker-10   |               |      ax25_find_cb() {
    0)   kworker-10   |   0.572 us    |        ax25cmp();
    0)   kworker-10   |   0.484 us    |        ax25cmp();
    0)   kworker-10   |   2.737 us    |      }
    0)   kworker-10   |               |      ax25_std_frame_in() {
    0)   kworker-10   |   0.569 us    |        ax25_decode();
    0)   kworker-10   |   0.607 us    |        ax25_validate_nr();
    0)   kworker-10   |               |        ax25_check_iframes_acked() {
    0)   kworker-10   |   4.171 us    |          ax25_frames_acked();
    0)   kworker-10   |               |          ax25_calculate_rtt() {
    0)   kworker-10   |   0.442 us    |            ax25_t1timer_running();
    0)   kworker-10   |   0.500 us    |            ax25_display_timer();
    0)   kworker-10   |   2.463 us    |          }
    0)   kworker-10   |   0.889 us    |          ax25_stop_t1timer();
    0)   kworker-10   |   1.559 us    |          ax25_start_t3timer();
    0)   kworker-10   | + 11.534 us   |        }
    0)   kworker-10   |               |        ax25_kick() {
    0)   kworker-10   |   0.480 us    |          ax25_kick.part.0();
    0)   kworker-10   |   1.467 us    |        }
    0)   kworker-10   | + 16.917 us   |      }
    0)   kworker-10   | + 27.062 us   |    }
    0)   kworker-10   | + 30.583 us   |  }
    1)   axwrapp-82   |               |  ax25_release() {
    1)   axwrapp-82   |   0.932 us    |    ax25_clear_queues();
    1)   axwrapp-82   |               |    ax25_send_control() {
    1)   axwrapp-82   |               |      ax25_transmit_buffer() {
    1)   axwrapp-82   |   0.516 us    |        ax25_addr_size();
    1)   axwrapp-82   |   0.484 us    |        ax25_addr_build();
    1)   axwrapp-82   |   0.451 us    |        ax25_fwd_dev();
    1)   axwrapp-82   | + 91.128 us   |      }
    1)   axwrapp-82   | + 94.596 us   |    }
    1)   axwrapp-82   |   1.554 us    |    ax25_stop_t2timer();
    1)   axwrapp-82   |   1.151 us    |    ax25_stop_t3timer();
    1)   axwrapp-82   |   0.758 us    |    ax25_stop_idletimer();
    1)   axwrapp-82   |   0.676 us    |    ax25_calculate_t1();
    1)   axwrapp-82   |   1.642 us    |    ax25_start_t1timer();
    1)   axwrapp-82   |               |  /* netdev_put: dev=ax0 */
    1)   axwrapp-82   | ! 123.696 us  |  }
    ------------------------------------------
    1)   axwrapp-82   =>   kworker-10  
    ------------------------------------------

    1)   kworker-10   |               |  ax25_kiss_rcv() {
    1)   kworker-10   |               |    ax25_rcv.isra.0() {
    1)   kworker-10   |   0.568 us    |      ax25_addr_parse();
    1)   kworker-10   |   0.428 us    |      ax25_addr_size();
    1)   kworker-10   |   0.504 us    |      ax25cmp();
    1)   kworker-10   |   0.453 us    |      ax25_digi_invert();
    1)   kworker-10   |               |      ax25_find_cb() {
    1)   kworker-10   |   0.425 us    |        ax25cmp();
    1)   kworker-10   |   0.429 us    |        ax25cmp();
    1)   kworker-10   |   2.511 us    |      }
    1)   kworker-10   |               |      ax25_std_frame_in() {
    1)   kworker-10   |   0.518 us    |        ax25_decode();
    1)   kworker-10   |               |        ax25_disconnect() {
    1)   kworker-10   |   0.840 us    |          ax25_stop_t1timer();
    1)   kworker-10   |   0.459 us    |          ax25_stop_t2timer();
    1)   kworker-10   |   0.444 us    |          ax25_stop_t3timer();
    1)   kworker-10   |   0.413 us    |          ax25_stop_idletimer();
    1)   kworker-10   |   1.123 us    |          ax25_link_failed();
    1)   kworker-10   |   6.202 us    |        }
    1)   kworker-10   |   0.428 us    |        ax25_kick();
    1)   kworker-10   |   9.214 us    |      }
    1)   kworker-10   | + 18.606 us   |    }
    1)   kworker-10   | + 21.675 us   |  }
    ------------------------------------------
    0)   kworker-10   =>    <idle>-0   
    ------------------------------------------

    0)    <idle>-0    |               |  ax25_heartbeat_expiry() {
    0)    <idle>-0    |               |    ax25_std_heartbeat_expiry() {
    0)    <idle>-0    |               |      ax25_destroy_socket() {
    0)    <idle>-0    |   0.202 us    |        ax25_cb_del();
    0)    <idle>-0    |   0.116 us    |        ax25_stop_heartbeat();
    0)    <idle>-0    |   0.100 us    |        ax25_stop_t1timer();
    0)    <idle>-0    |   0.101 us    |        ax25_stop_t2timer();
    0)    <idle>-0    |   0.158 us    |        ax25_stop_t3timer();
    0)    <idle>-0    |   0.101 us    |        ax25_stop_idletimer();
    0)    <idle>-0    |   0.321 us    |        ax25_clear_queues();
    0)    <idle>-0    |   2.483 us    |      }
    0)    <idle>-0    |   0.531 us    |      ax25_free_sock();
    0)    <idle>-0    |   4.912 us    |    }
    0)    <idle>-0    |   6.545 us    |  }

[1]: https://gist.github.com/larsks/b658c0bd766648b16c31c8ed0fc1dc1f#file-0001-trace-netdev_hold-and-netdev_put-patch
[2]: https://gist.github.com/larsks/b658c0bd766648b16c31c8ed0fc1dc1f#file-trace1-inbound-txt
[3]: https://github.com/torvalds/linux/blob/7367539ad4b0f8f9b396baf02110962333719a48/net/ax25/af_ax25.c#L1430
[4]: https://gist.github.com/larsks/b658c0bd766648b16c31c8ed0fc1dc1f#file-trace2-inbound-txt

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

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Lars Kellogg-Stedman]

On Sat, May 04, 2024 at 06:16:14PM GMT, Lars Kellogg-Stedman wrote:
> My original patch corrected this by adding the call to netdev_hold()
> right next to the ax25_cb_add() in ax25_rcv(), which solves this
> problem. If it seems weird to have this login in ax25_rcv, we could move
> it to ax25_accept, right around line 1430 [3]; that would look
> something like:

The same patch applies cleanly against the Raspberry Pi 6.6.30 kernel,
and clears up the frequeny crashes I was experiencing in that
environment as well.

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

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

On Mon, May 06, 2024 at 11:18:06PM -0400, Lars Kellogg-Stedman wrote:
> On Sat, May 04, 2024 at 06:16:14PM GMT, Lars Kellogg-Stedman wrote:
> > My original patch corrected this by adding the call to netdev_hold()
> > right next to the ax25_cb_add() in ax25_rcv(), which solves this
> > problem. If it seems weird to have this login in ax25_rcv, we could move
> > it to ax25_accept, right around line 1430 [3]; that would look
> > something like:
> 
> The same patch applies cleanly against the Raspberry Pi 6.6.30 kernel,
> and clears up the frequeny crashes I was experiencing in that
> environment as well.

I have reviewed this code some more.  My theory is:

ax25_dev_device_up() <- sets refcount to 1
ax25_dev_device_down() <- set refcount to 0 and frees it

If the refcount is not 1 at ax25_dev_device_down() then something is
screwed up.  So why do we even need more refcounting than that?  But
apparently we do.  I don't really understand networking that well so
maybe we can have lingering connections after the device is down.

So the next rule is if we set ax25->ax25_dev from NULL to non-NULL then
bump the refcount and decrement it if we set it back to NULL or we free
ax25. Right now that's happening in ax25_bind() and ax25_release().  And
also in ax25_kill_by_device() but not consistently.

But it needs to happen *everywhere* we set ax25->ax25_dev and we need to
decrement it when ax25 is freed in ax25_cb_put().  My patch is similar
to yours in that every ax25_rcv() will now bump the reference through
calling ax25_fillin_cb() or ax25_make_new().  The send path also bumps
the reference.

There are a few questions I don't know how to answer.  I added two
-EBUSY paths to this patch.  I'm not sure if this is correct.
Second, I don't understand the netdev_put(ax25_dev->dev, &s->dev_tracker);
stuff.  Maybe that should be done in ax25_dev_hold/put().

This patch might not work because of the netdev_hold/put() thing...

I used the Smatch cross function database to show where ax25->ax25_dev
is set to NULL/non-NULL.

$ smdb.py where ax25_cb ax25_dev | grep -v "min-max"
net/ax25/ax25_route.c          | ax25_rt_autobind               | (struct ax25_cb)->ax25_dev | 0-u64max
net/ax25/af_ax25.c             | ax25_kill_by_device            | (struct ax25_cb)->ax25_dev | 0-u64max
net/ax25/af_ax25.c             | ax25_fillin_cb                 | (struct ax25_cb)->ax25_dev | 0-u64max
net/ax25/af_ax25.c             | ax25_setsockopt                | (struct ax25_cb)->ax25_dev | 0-u64max
net/ax25/af_ax25.c             | ax25_make_new                  | (struct ax25_cb)->ax25_dev | 0-u64max
net/ax25/af_ax25.c             | ax25_bind                      | (struct ax25_cb)->ax25_dev | 4096-ptr_max
net/ax25/ax25_in.c             | ax25_rcv                       | (struct ax25_cb)->ax25_dev | 0-u64max
net/ax25/ax25_out.c            | ax25_send_frame                | (struct ax25_cb)->ax25_dev | 0-u64max

regards,
dan carpenter

diff --git a/include/net/ax25.h b/include/net/ax25.h
index eb9cee8252c8..2cc94352b13d 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -275,25 +275,30 @@ static inline struct ax25_cb *sk_to_ax25(const struct sock *sk)
 #define ax25_cb_hold(__ax25) \
 	refcount_inc(&((__ax25)->refcount))
 
-static __inline__ void ax25_cb_put(ax25_cb *ax25)
+static inline ax25_dev *ax25_dev_hold(ax25_dev *ax25_dev)
 {
-	if (refcount_dec_and_test(&ax25->refcount)) {
-		kfree(ax25->digipeat);
-		kfree(ax25);
-	}
+	if (ax25_dev)
+		refcount_inc(&ax25_dev->refcount);
+	return ax25_dev;
 }
 
-static inline void ax25_dev_hold(ax25_dev *ax25_dev)
+static inline void ax25_dev_put(ax25_dev *ax25_dev)
 {
-	refcount_inc(&ax25_dev->refcount);
+	if (ax25_dev && refcount_dec_and_test(&ax25_dev->refcount)) {
+		kfree(ax25_dev);
+	}
 }
 
-static inline void ax25_dev_put(ax25_dev *ax25_dev)
+static __inline__ void ax25_cb_put(ax25_cb *ax25)
 {
-	if (refcount_dec_and_test(&ax25_dev->refcount)) {
-		kfree(ax25_dev);
+	if (refcount_dec_and_test(&ax25->refcount)) {
+		if (ax25->ax25_dev)
+			ax25_dev_put(ax25->ax25_dev);
+		kfree(ax25->digipeat);
+		kfree(ax25);
 	}
 }
+
 static inline __be16 ax25_type_trans(struct sk_buff *skb, struct net_device *dev)
 {
 	skb->dev      = dev;
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 9169efb2f43a..4d1ab296d52c 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -92,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
 				spin_unlock_bh(&ax25_list_lock);
 				ax25_disconnect(s, ENETUNREACH);
 				s->ax25_dev = NULL;
+				ax25_dev_put(ax25_dev);
 				ax25_cb_del(s);
 				spin_lock_bh(&ax25_list_lock);
 				goto again;
@@ -101,11 +102,8 @@ static void ax25_kill_by_device(struct net_device *dev)
 			lock_sock(sk);
 			ax25_disconnect(s, ENETUNREACH);
 			s->ax25_dev = NULL;
-			if (sk->sk_socket) {
-				netdev_put(ax25_dev->dev,
-					   &s->dev_tracker);
-				ax25_dev_put(ax25_dev);
-			}
+			netdev_put(ax25_dev->dev, &s->dev_tracker);
+			ax25_dev_put(ax25_dev);
 			ax25_cb_del(s);
 			release_sock(sk);
 			spin_lock_bh(&ax25_list_lock);
@@ -496,6 +494,7 @@ void ax25_fillin_cb(ax25_cb *ax25, ax25_dev *ax25_dev)
 	ax25->ax25_dev = ax25_dev;
 
 	if (ax25->ax25_dev != NULL) {
+		ax25_dev_hold(ax25->ax25_dev);
 		ax25_fillin_cb_from_dev(ax25, ax25_dev);
 		return;
 	}
@@ -685,6 +684,11 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 			break;
 		}
 
+		if (ax25->ax25_dev) {
+			rtnl_unlock();
+			res = -EBUSY;
+			break;
+		}
 		ax25->ax25_dev = ax25_dev_ax25dev(dev);
 		if (!ax25->ax25_dev) {
 			rtnl_unlock();
@@ -961,7 +965,7 @@ struct sock *ax25_make_new(struct sock *osk, struct ax25_dev *ax25_dev)
 	ax25->paclen  = oax25->paclen;
 	ax25->window  = oax25->window;
 
-	ax25->ax25_dev    = ax25_dev;
+	ax25->ax25_dev    = ax25_dev_hold(ax25_dev);
 	ax25->source_addr = oax25->source_addr;
 
 	if (oax25->digipeat != NULL) {
@@ -995,6 +999,11 @@ static int ax25_release(struct socket *sock)
 	sock_orphan(sk);
 	ax25 = sk_to_ax25(sk);
 	ax25_dev = ax25->ax25_dev;
+	/*
+	 * The ax25_destroy_socket() function decrements the reference but we
+	 * need to keep a reference until the end of the function.
+	 */
+	ax25_dev_hold(ax25_dev);
 
 	if (sk->sk_type == SOCK_SEQPACKET) {
 		switch (ax25->state) {
@@ -1147,6 +1156,12 @@ static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 
 	if (ax25_dev) {
 		ax25_fillin_cb(ax25, ax25_dev);
+		/*
+		 * both ax25_addr_ax25dev() and ax25_fillin_cb() take a
+		 * reference but we only want to take one reference so drop
+		 * the extra reference.
+		 */
+		ax25_dev_put(ax25_dev);
 		netdev_hold(ax25_dev->dev, &ax25->dev_tracker, GFP_ATOMIC);
 	}
 
diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c
index b7c4d656a94b..d7f6d9f4f20c 100644
--- a/net/ax25/ax25_route.c
+++ b/net/ax25/ax25_route.c
@@ -406,6 +406,10 @@ int ax25_rt_autobind(ax25_cb *ax25, ax25_address *addr)
 		ax25_route_lock_unuse();
 		return -EHOSTUNREACH;
 	}
+	if (ax25->ax25_dev) {
+		err = -EBUSY;
+		goto put;
+	}
 	if ((ax25->ax25_dev = ax25_dev_ax25dev(ax25_rt->dev)) == NULL) {
 		err = -EHOSTUNREACH;
 		goto put;

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[duoming]

On Tue, 7 May 2024 11:08:14 +0300 Dan Carpenter wrote:
> I have reviewed this code some more.  My theory is:
> 
> ax25_dev_device_up() <- sets refcount to 1
> ax25_dev_device_down() <- set refcount to 0 and frees it
> 
> If the refcount is not 1 at ax25_dev_device_down() then something is
> screwed up.  So why do we even need more refcounting than that?  But
> apparently we do.  I don't really understand networking that well so
> maybe we can have lingering connections after the device is down.

We do need more reference count. Because there is a race condition 
between ax25_bind() and the cleanup routine.

The cleanup routine is consisted of three parts: ax25_kill_by_device(),
ax25_rt_device_down() and ax25_dev_device_down(). The ax25_kill_by_device()
is used to cleanup the connections and the ax25_dev_device_down() is used
to cleanup the device. If we call ax25_bind() and ax25_connect() between
the window of ax25_kill_by_device() and ax25_dev_device_down(), the ax25_dev
is freed in ax25_dev_device_down(). When we call ax25_release() to release
the connections, the UAF bugs will happen. 

Best regards,
Duoming Zhou

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

On Tue, May 07, 2024 at 05:04:05PM +0800, duoming@zju.edu.cn wrote:
> On Tue, 7 May 2024 11:08:14 +0300 Dan Carpenter wrote:
> > I have reviewed this code some more.  My theory is:
> > 
> > ax25_dev_device_up() <- sets refcount to 1
> > ax25_dev_device_down() <- set refcount to 0 and frees it
> > 
> > If the refcount is not 1 at ax25_dev_device_down() then something is
> > screwed up.  So why do we even need more refcounting than that?  But
> > apparently we do.  I don't really understand networking that well so
> > maybe we can have lingering connections after the device is down.
> 
> We do need more reference count. Because there is a race condition 
> between ax25_bind() and the cleanup routine.
> 
> The cleanup routine is consisted of three parts: ax25_kill_by_device(),
> ax25_rt_device_down() and ax25_dev_device_down(). The ax25_kill_by_device()
> is used to cleanup the connections and the ax25_dev_device_down() is used
> to cleanup the device. If we call ax25_bind() and ax25_connect() between
> the window of ax25_kill_by_device() and ax25_dev_device_down(), the ax25_dev
> is freed in ax25_dev_device_down(). When we call ax25_release() to release
> the connections, the UAF bugs will happen. 

If that's the case, couldn't we do something much simpler where we set a
"no_more_binds = true;" in ax25_kill_by_device() and refuse to bind if
we've started that process?  Or we could take the ax25_dev's off the
ax25_dev_list and put it onto a ax25_dev_list_killed list.  The the
ax25_dev_list_killed would only be used in ax25_dev_device_down().

(I'm just throwing out ideas).

regards,
dan carpenter

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[duoming]

On Tue, 7 May 2024 11:08:14 +0300 Dan Carpenter wrote:
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 9169efb2f43a..4d1ab296d52c 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -92,6 +92,7 @@ static void ax25_kill_by_device(struct net_device *dev)
>  				spin_unlock_bh(&ax25_list_lock);
>  				ax25_disconnect(s, ENETUNREACH);
>  				s->ax25_dev = NULL;
> +				ax25_dev_put(ax25_dev);
>  				ax25_cb_del(s);
>  				spin_lock_bh(&ax25_list_lock);
>  				goto again;
> @@ -101,11 +102,8 @@ static void ax25_kill_by_device(struct net_device *dev)
>  			lock_sock(sk);
>  			ax25_disconnect(s, ENETUNREACH);
>  			s->ax25_dev = NULL;
> -			if (sk->sk_socket) {
> -				netdev_put(ax25_dev->dev,
> -					   &s->dev_tracker);
> -				ax25_dev_put(ax25_dev);
> -			}
> +			netdev_put(ax25_dev->dev, &s->dev_tracker);
> +			ax25_dev_put(ax25_dev);

We should not decrease the refcount without checking "if (sk->sk_socket)", because 
there is a race condition between ax25_kill_by_device() and ax25_release(), if we
decrease the refcount in ax25_release(), we should not decrease it here, otherwise 
the refcount underflow will happen.

Best regards,
Duoming Zhou

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Lars Kellogg-Stedman]

Dan,

Apologies, I missed the patch when you first posted it. Thanks for the
pointer.

On Tue, May 07, 2024 at 11:08:14AM GMT, Dan Carpenter wrote:
> This patch might not work because of the netdev_hold/put() thing...

I think that's the case. It's the netdev_hold/netdev_put imbalance that
is causing the kernel issues; with your patch applied, we still see a
failure in ax25_release:

    refcount_t: decrement hit 0; leaking memory.

The patch I've posted resolves this issue and runs without any errors.

The complete trace is:

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 1 PID: 88 at lib/refcount.c:31 refcount_warn_saturate+0x109/0x120

CPU: 1 PID: 88 Comm: axwrapper Not tainted 6.9.0-ax25-09869-g6d35085a1f38 #140
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
RIP: 0010:refcount_warn_saturate+0x109/0x120
Code: f2 33 82 c6 05 34 62 f2 00 01 e8 22 14 9d ff 0f 0b 5d c3 cc cc cc cc 48 c7 c7 58 f2 33 82 c6 05 17 62 f2 00 01 e8 07 14 9d ff <0f> 0b 5d c3 cc cc cc cc 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40
RSP: 0018:ffffc90000447d00 EFLAGS: 00010292
RAX: 000000000000002c RBX: ffff888101142510 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffffc90000447b88 RDI: 00000000ffffefff
The system is gRBP: ffffc90000447d00 R08: 00000000ffffefff R09: ffffffff824a4b88
R10: ffffffff8244cbe0 R11: ffffc90000447ad8 R12: 0000000000000000
oing down NOW!R13: ffffc90000447d18 R14: ffff888101142000 R15: ffff88810222a0c0

FS:  0000000000000000(0000) GS:ffff88813bd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Sent SIGTERM toCR2: 000055d73c5f5040 CR3: 000000000242c000 CR4: 00000000000006b0
Call Trace:
 all processes <TASK>

 ? show_regs.part.0+0x22/0x30
 ? show_regs.cold+0x8/0xd
 ? refcount_warn_saturate+0x109/0x120
 ? __warn.cold+0x97/0xd5
 ? refcount_warn_saturate+0x109/0x120
 ? report_bug+0x114/0x160
 ? console_unlock+0x55/0xd0
 ? handle_bug+0x42/0x80
 ? exc_invalid_op+0x1c/0x70
 ? asm_exc_invalid_op+0x1f/0x30
 ? refcount_warn_saturate+0x109/0x120
 ref_tracker_free+0x163/0x170
 ax25_release+0x129/0x3c0
 sock_close+0x45/0xb0
 __fput+0x94/0x2a0
 ____fput+0x12/0x20
 task_work_run+0x61/0x90
 do_exit+0x2f5/0x9f0
 ? handle_mm_fault+0x197/0x300
 do_group_exit+0x38/0x90
 __x64_sys_exit_group+0x1c/0x20
 x64_sys_call+0x1269/0x1d00
 do_syscall_64+0x55/0x120
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7f8e7711abce
Code: Unable to access opcode bytes at 0x7f8e7711aba4.
RSP: 002b:00007ffed9c905d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f8e7711abce
RDX: 00007f8e7711ae66 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 00007ffed9c90628 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000055d73c5f2030
R13: 00007ffed9c90658 R14: 0000000000000000 R15: 00007ffed9c90620
 </TASK>
---[ end trace 0000000000000000 ]---

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

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

On Fri, May 03, 2024 at 11:36:37PM +0300, Dan Carpenter wrote:
> Could you test this diff?
> 
> regards,
> dan carpenter
> 
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 558e158c98d0..a7f96a4ceff4 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -1129,8 +1129,10 @@ static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	/*
>  	 * User already set interface with SO_BINDTODEVICE
>  	 */
> -	if (ax25->ax25_dev != NULL)
> +	if (ax25->ax25_dev != NULL) {
> +		ax25_dev_hold(ax25->ax25_dev);
>  		goto done;
> +	}
>  
>  	if (addr_len > sizeof(struct sockaddr_ax25) && addr->fsa_ax25.sax25_ndigis == 1) {
>  		if (ax25cmp(&addr->fsa_digipeater[0], &null_ax25_address) != 0 &&

This commit is wrong.

regards,
dan carpenter

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[duoming]

On Wed, 1 May 2024 21:29:16 -0400 Lars Kellogg-Stedman wrote:
> Assume we have the following two interfaces configured on a system:
> 
>     $ cat /etc/ax25/axports
>     udp0 test0-0 9600 255 2 axudp0
>     udp1 test0-1 9600 255 2 axudp1
> 
> And we have ax25d listening on both interfaces:
> 
>     [udp0]
>     default  * * * * * *  - root  /usr/sbin/axwrapper axwrapper -- /bin/sh sh /etc/ax25/example-output.sh
>     [udp1]
>     default  * * * * * *  - root  /usr/sbin/axwrapper axwrapper -- /bin/sh sh /etc/ax25/example-output.sh
> 
> Using the 'ax-devs' and 'ax-sockets' gdb commands shown at the end of
> this message, we start with:
> 
>     (gdb) ax-devs
>     ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
>     ax0 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
>     (gdb) ax-sockets
>     0xffff8881002b6800 if:ax1 state:0 refcnt:2 dev_tracker:0xffff888100ded200
>     0xffff888101ac4e00 if:ax0 state:0 refcnt:2 dev_tracker:0xffff888100dec4c0
> 
> We initiate a connection from ax0 to ax1:
> 
>     call -r udp0 test0-1
> 
> When we first enter ax25_rcv, we have:
> 
>     (gdb) ax-devs
>     ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
>     ax0 ax_refcnt:3 dev_refcnt:10 dev_untracked:1 dev_notrack:1
>     (gdb) ax-sockets
>     0xffff888101ac8000 if:ax0 state:1 refcnt:2 dev_tracker:0xffff888100dedb80
>     0xffff8881002b6800 if:ax1 state:0 refcnt:2 dev_tracker:0xffff888100ded200
>     0xffff888101ac4e00 if:ax0 state:0 refcnt:2 dev_tracker:0xffff888100dec4c0
> 
> After we reach line 413 (in net/ax25/ax25_in.c) and add a new control
> block:
> 
>     ax25_cb_add(ax25)
> 
> We have:
> 
>     (gdb) ax-devs
>     ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
>     ax0 ax_refcnt:3 dev_refcnt:10 dev_untracked:1 dev_notrack:1
>     (gdb) ax-sockets
>     0xffff88810245ac00 if:ax1 state:3 refcnt:2 dev_tracker:0x0 <fixed_percpu_data>
>     0xffff88810245ba00 if:ax0 state:1 refcnt:2 dev_tracker:0xffff88810136c800
>     0xffff888100c79e00 if:ax1 state:0 refcnt:2 dev_tracker:0xffff88810136c6e0
>     0xffff8881018e9800 if:ax0 state:0 refcnt:2 dev_tracker:0xffff88810170c860
> 
> Note that (a) ax25->dev_tracker is NULL, and (b) we have incremeted the
> refcount on ax0 (the source interface), but not on ax1 (the destination
> interface). When we call ax25_release for this control block, we get to:
> 
>     netdev_put(ax25_dev->dev, &ax25->dev_tracker);
>     ax25_dev_put(ax25_dev);
> 
> With:
> 
>     (gdb) ax-devs
>     ax1 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
>     ax0 ax_refcnt:3 dev_refcnt:10 dev_untracked:1 dev_notrack:1
> 
> After the calls to netdev_put() and ax25_dev_put(), we have:
> 
>     (gdb) ax-devs
>     ax1 ax_refcnt:1 dev_refcnt:8 dev_untracked:-1073741824 dev_notrack:1
>     ax0 ax_refcnt:2 dev_refcnt:9 dev_untracked:1 dev_notrack:1
> 
> You can see that (a) ax25_dev->dev->refcnt_tracker->untracked is now
> invalid, and ax25_dev->dev->dev_refcnt is in trouble: it decrements by
> one for each closed connection, even though it was never incremented
> when we accepted the connection. The underflow in
> ...refcnt_tracker->untracked yields the traceback with:
> 
>     refcount_t: decrement hit 0; leaking memory.
> 
> Additional connections will eventually trigger more problems; we will
> ultimately underflow ax25_dev->dev->dev_refcnt, but we may also run into
> memory corruption because of the invalid tracker data, resulting in:
> 
>     BUG: unable to handle page fault for address: 00000010000003b0

Do you know how to trigger this bug? Could you share the POC?

Best regards,
Duoming Zhou

Re: [PATCH net] ax25: Fix refcount leak issues of ax25_dev

[Lars Kellogg-Stedman]

On Wed, May 15, 2024 at 05:52:25PM GMT, duoming@zju.edu.cn wrote:
> On Wed, 1 May 2024 21:29:16 -0400 Lars Kellogg-Stedman wrote:
> Do you know how to trigger this bug? Could you share the POC?

[I've trimmed the Cc: list a bit]

Yes, triggering this issue is trivial. As far as I can tell, it happens
for *any* incoming connection.  I use a script [1] to up a simple test
environment on a clean system like this:

    bash setup-ax25.sh node0

This will configure a pair of axudp interfaces (one for `node0-0` and
one for `node-1`), and configure ax25d to listen on both interfaces.
Then just establish a connection:

    axcall -r udp0 node0-1

This will immediately trigger the problem, and subsequently the system
will be unable to reboot because of the "waiting for <device> to become
free" issue.

You can see from the traces I posted in a previous email that the
problem is pretty clear -- we never call netdev_hold when accepting a
new connection, but we *do* call netdev_put in ax25_release, leading to
the problem.

[1]: https://github.com/larsks/ax25-debugging/blob/main/scripts/setup-ax25.sh

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

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Sun, May 19, 2024 at 07:52:41PM GMT, Chris Maness wrote:
> Since you guys have been working on this, I have started playing around
> with slackware-current to check on your progress.  I setup LinFBB with one
> pseudo tty  port generated with ax25ipd.  It does release the socket after
> connection, and worked for a couple of hours.  However, it crashed hard
> with no terminal dump when I was forwarding some bulls over AX.25 over UDP
> to the ax25ipd.  This does involve the AX.25 stack as FBB needs an ax0 port.

Chris,

At the moment, the Linux ax.25 stack is unusable for handling incoming
connections (outbound connections seem to be stable).

I posted a patch to this list at the end of April that has completely
resolved the ax.25 crashing problems for me. I'm running it on x86_64
systems with kernel 6.9.0, and on Raspberry Pi systems running 6.6.30.

Dan was unhappy with the patch, and Duoming has been silent on this
topic, so I'm not sure how to proceed. I am confident that there is a
"more correct" solution to this problem, but I am also confident that
this patch corrects a real issue; without a more experienced developer
either suggesting changes to this patch or submitting their own I don't
know if this will move forward. Duoming indicated some interest  in
the issue on 5/15 [1], but I haven't heard anything since then.

I've included the current version of the patch in this email. If you
have the chance to try it out, I'm curious to know whether or not it
solves your problems.

Cheers,

-- Lars

[1] https://marc.info/?l=linux-kernel&m=171576662414653&w=2

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

When closing a socket in ax25_release(), we call netdev_put() to decrease
the refcount on the ax.25 device. However, the execution path for accepting
an incoming connection never calls ax25_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 ax1 to become free. Usage count = 0

The attached patch corrects these issues by ensuring that we call
netdev_hold() and ax25_dev_hold() in ax25_accept().

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 9169efb2f43..5aa6e5c3495 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, int flags,
 	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, int flags,
 	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: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

On Mon, May 20, 2024 at 7:42 AM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
>
> On Sun, May 19, 2024 at 07:52:41PM GMT, Chris Maness wrote:
> > Since you guys have been working on this, I have started playing around
> > with slackware-current to check on your progress.  I setup LinFBB with one
> > pseudo tty  port generated with ax25ipd.  It does release the socket after
> > connection, and worked for a couple of hours.  However, it crashed hard
> > with no terminal dump when I was forwarding some bulls over AX.25 over UDP
> > to the ax25ipd.  This does involve the AX.25 stack as FBB needs an ax0 port.
>
> Chris,
>
> At the moment, the Linux ax.25 stack is unusable for handling incoming
> connections (outbound connections seem to be stable).
>
> I posted a patch to this list at the end of April that has completely
> resolved the ax.25 crashing problems for me. I'm running it on x86_64
> systems with kernel 6.9.0, and on Raspberry Pi systems running 6.6.30.
>
> Dan was unhappy with the patch, and Duoming has been silent on this
> topic, so I'm not sure how to proceed. I am confident that there is a
> "more correct" solution to this problem, but I am also confident that
> this patch corrects a real issue; without a more experienced developer
> either suggesting changes to this patch or submitting their own I don't
> know if this will move forward. Duoming indicated some interest  in
> the issue on 5/15 [1], but I haven't heard anything since then.
>
> I've included the current version of the patch in this email. If you
> have the chance to try it out, I'm curious to know whether or not it
> solves your problems.
>
> Cheers,
>
> -- Lars
>
> [1] https://marc.info/?l=linux-kernel&m=171576662414653&w=2
>
> >8------------------------------------------------------8<
>
> When closing a socket in ax25_release(), we call netdev_put() to decrease
> the refcount on the ax.25 device. However, the execution path for accepting
> an incoming connection never calls ax25_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 ax1 to become free. Usage count = 0
>
> The attached patch corrects these issues by ensuring that we call
> netdev_hold() and ax25_dev_hold() in ax25_accept().
>
> 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 9169efb2f43..5aa6e5c3495 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, int flags,
>         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, int flags,
>         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

That would make sense, because I triggered the crash by initiating a
forward session from an external BBS connecting IN with my listening
LinFBB BBS software instance running inside the Slackware-current
(6.9.1) machine.  It crashed immediately.

I will patch and rebuild the kernel.  Fortunately, that is easy to do
with Slackware as they run a stock kernel.

-Chris KQ6UP

-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

Your patch seems to have fixed the issue so far.  It would require a
longer stress test, but shot some pretty
big bulletins at it, and so far so good.  How come this patch is not
included in the mainline source?

-Chris KQ6UP

On Mon, May 20, 2024 at 7:42 AM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
>
> On Sun, May 19, 2024 at 07:52:41PM GMT, Chris Maness wrote:
> > Since you guys have been working on this, I have started playing around
> > with slackware-current to check on your progress.  I setup LinFBB with one
> > pseudo tty  port generated with ax25ipd.  It does release the socket after
> > connection, and worked for a couple of hours.  However, it crashed hard
> > with no terminal dump when I was forwarding some bulls over AX.25 over UDP
> > to the ax25ipd.  This does involve the AX.25 stack as FBB needs an ax0 port.
>
> Chris,
>
> At the moment, the Linux ax.25 stack is unusable for handling incoming
> connections (outbound connections seem to be stable).
>
> I posted a patch to this list at the end of April that has completely
> resolved the ax.25 crashing problems for me. I'm running it on x86_64
> systems with kernel 6.9.0, and on Raspberry Pi systems running 6.6.30.
>
> Dan was unhappy with the patch, and Duoming has been silent on this
> topic, so I'm not sure how to proceed. I am confident that there is a
> "more correct" solution to this problem, but I am also confident that
> this patch corrects a real issue; without a more experienced developer
> either suggesting changes to this patch or submitting their own I don't
> know if this will move forward. Duoming indicated some interest  in
> the issue on 5/15 [1], but I haven't heard anything since then.
>
> I've included the current version of the patch in this email. If you
> have the chance to try it out, I'm curious to know whether or not it
> solves your problems.
>
> Cheers,
>
> -- Lars
>
> [1] https://marc.info/?l=linux-kernel&m=171576662414653&w=2
>
> >8------------------------------------------------------8<
>
> When closing a socket in ax25_release(), we call netdev_put() to decrease
> the refcount on the ax.25 device. However, the execution path for accepting
> an incoming connection never calls ax25_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 ax1 to become free. Usage count = 0
>
> The attached patch corrects these issues by ensuring that we call
> netdev_hold() and ax25_dev_hold() in ax25_accept().
>
> 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 9169efb2f43..5aa6e5c3495 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, int flags,
>         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, int flags,
>         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: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Mon, May 20, 2024 at 05:11:39PM GMT, Chris Maness wrote:
> Your patch seems to have fixed the issue so far.

That's great! I'm glad it helped.

> How come this patch is not included in the mainline source?

Well, partly because you're the first person to confirm that it works
for someone besides me :). Dan (Cross) has offered to test it out as
well; if I can get a couple of confirmations that it's working, I'll
probably go ahead and submit it to netdev.

The bigger issue has been that there are very few people interested in
the ax.25 support in the kernel; there's not a real maintainer, so it's
difficult to get code approved. There were some concerns expressed that
maybe this isn't the *correct* fix, but I would argue that even if it's
not the most correct fix we should try to get it in anyway, since
otherwise ax.25 is completely broken.

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

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

Yes, I would say that it bombed pretty quickly without the patch, so A
patch is better than NO patch -- and often we are always our own worst
critic.  I have NETROM working too now in kernel space.  Seems to be
ok.

Thanks,
Chris Maness
-Sent from my iPhone


On Mon, May 20, 2024 at 5:48 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
>
> On Mon, May 20, 2024 at 05:11:39PM GMT, Chris Maness wrote:
> > Your patch seems to have fixed the issue so far.
>
> That's great! I'm glad it helped.
>
> > How come this patch is not included in the mainline source?
>
> Well, partly because you're the first person to confirm that it works
> for someone besides me :). Dan (Cross) has offered to test it out as
> well; if I can get a couple of confirmations that it's working, I'll
> probably go ahead and submit it to netdev.
>
> The bigger issue has been that there are very few people interested in
> the ax.25 support in the kernel; there's not a real maintainer, so it's
> difficult to get code approved. There were some concerns expressed that
> maybe this isn't the *correct* fix, but I would argue that even if it's
> not the most correct fix we should try to get it in anyway, since
> otherwise ax.25 is completely broken.
>
> --
> Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
> http://blog.oddbit.com/                | N1LKS



-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Dan Cross]

On Mon, May 20, 2024 at 9:06 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
> On Mon, May 20, 2024 at 05:11:39PM GMT, Chris Maness wrote:
> > Your patch seems to have fixed the issue so far.
>
> That's great! I'm glad it helped.
>
> > How come this patch is not included in the mainline source?
>
> Well, partly because you're the first person to confirm that it works
> for someone besides me :). Dan (Cross) has offered to test it out as
> well; if I can get a couple of confirmations that it's working, I'll
> probably go ahead and submit it to netdev.

I am running this as of today, and so far so good.

> The bigger issue has been that there are very few people interested in
> the ax.25 support in the kernel; there's not a real maintainer, so it's
> difficult to get code approved. There were some concerns expressed that
> maybe this isn't the *correct* fix, but I would argue that even if it's
> not the most correct fix we should try to get it in anyway, since
> otherwise ax.25 is completely broken.

Agreed: what's there now is objectively broken. Please, let's not let
the perfect be the enemy of the good here.

        - Dan C.

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

Ok, the only thing not working so far is UROnode in this system.  I am
thinking it needs to be updated to the newer kernel.  I am getting:

ERROR; connect_to: bind: Cannot assign requested address

When I try to connect to a NETROM node from UROnode (AX.25 only works tho).

I can connect from LinFBB with NETROM directly to other nodes using my
NETROM port in LinFBB.

-Chris KQ6UP


On Mon, May 20, 2024 at 7:12 PM Dan Cross <crossd@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 9:06 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
> > On Mon, May 20, 2024 at 05:11:39PM GMT, Chris Maness wrote:
> > > Your patch seems to have fixed the issue so far.
> >
> > That's great! I'm glad it helped.
> >
> > > How come this patch is not included in the mainline source?
> >
> > Well, partly because you're the first person to confirm that it works
> > for someone besides me :). Dan (Cross) has offered to test it out as
> > well; if I can get a couple of confirmations that it's working, I'll
> > probably go ahead and submit it to netdev.
>
> I am running this as of today, and so far so good.
>
> > The bigger issue has been that there are very few people interested in
> > the ax.25 support in the kernel; there's not a real maintainer, so it's
> > difficult to get code approved. There were some concerns expressed that
> > maybe this isn't the *correct* fix, but I would argue that even if it's
> > not the most correct fix we should try to get it in anyway, since
> > otherwise ax.25 is completely broken.
>
> Agreed: what's there now is objectively broken. Please, let's not let
> the perfect be the enemy of the good here.
>
>         - Dan C.



-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Dave Hibberd]

Can you use other libax25 dependent packages or is it only Uronode?

axcall for netrom would be a reasonable starter to see if it's a libax25 problem or not. 

LinFBB, from what I remember, uses its own internal state machine/etc once it's bound to the socket.

Cheers,

-- 
  Hibby
  Debian Developer
  Packet Radioist
  MM0RFN

On Tue, 21 May 2024, at 4:07 AM, Chris Maness wrote:
> Ok, the only thing not working so far is UROnode in this system.  I am
> thinking it needs to be updated to the newer kernel.  I am getting:
>
> ERROR; connect_to: bind: Cannot assign requested address
>
> When I try to connect to a NETROM node from UROnode (AX.25 only works tho).
>
> I can connect from LinFBB with NETROM directly to other nodes using my
> NETROM port in LinFBB.
>
> -Chris KQ6UP
>
>
> On Mon, May 20, 2024 at 7:12 PM Dan Cross <crossd@gmail.com> wrote:
>>
>> On Mon, May 20, 2024 at 9:06 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
>> > On Mon, May 20, 2024 at 05:11:39PM GMT, Chris Maness wrote:
>> > > Your patch seems to have fixed the issue so far.
>> >
>> > That's great! I'm glad it helped.
>> >
>> > > How come this patch is not included in the mainline source?
>> >
>> > Well, partly because you're the first person to confirm that it works
>> > for someone besides me :). Dan (Cross) has offered to test it out as
>> > well; if I can get a couple of confirmations that it's working, I'll
>> > probably go ahead and submit it to netdev.
>>
>> I am running this as of today, and so far so good.
>>
>> > The bigger issue has been that there are very few people interested in
>> > the ax.25 support in the kernel; there's not a real maintainer, so it's
>> > difficult to get code approved. There were some concerns expressed that
>> > maybe this isn't the *correct* fix, but I would argue that even if it's
>> > not the most correct fix we should try to get it in anyway, since
>> > otherwise ax.25 is completely broken.
>>
>> Agreed: what's there now is objectively broken. Please, let's not let
>> the perfect be the enemy of the good here.
>>
>>         - Dan C.
>
>
>
> -- 
> Thanks,
> Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

netrom_call has not worked for ages.  I am using VE7FET's forked
userspace, not the official.  I could build another
system with the official userspace I suppose.  My understanding is
that VE7FET fixed some bugs, and I have had
better luck with it for the most part.

-Chris KQ6UP

On Tue, May 21, 2024 at 4:48 AM Dave Hibberd <hibby@debian.org> wrote:
>
> Can you use other libax25 dependent packages or is it only Uronode?
>
> axcall for netrom would be a reasonable starter to see if it's a libax25 problem or not.
>
> LinFBB, from what I remember, uses its own internal state machine/etc once it's bound to the socket.
>
> Cheers,
>
> --
>   Hibby
>   Debian Developer
>   Packet Radioist
>   MM0RFN
>
> On Tue, 21 May 2024, at 4:07 AM, Chris Maness wrote:
> > Ok, the only thing not working so far is UROnode in this system.  I am
> > thinking it needs to be updated to the newer kernel.  I am getting:
> >
> > ERROR; connect_to: bind: Cannot assign requested address
> >
> > When I try to connect to a NETROM node from UROnode (AX.25 only works tho).
> >
> > I can connect from LinFBB with NETROM directly to other nodes using my
> > NETROM port in LinFBB.
> >
> > -Chris KQ6UP
> >
> >
> > On Mon, May 20, 2024 at 7:12 PM Dan Cross <crossd@gmail.com> wrote:
> >>
> >> On Mon, May 20, 2024 at 9:06 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
> >> > On Mon, May 20, 2024 at 05:11:39PM GMT, Chris Maness wrote:
> >> > > Your patch seems to have fixed the issue so far.
> >> >
> >> > That's great! I'm glad it helped.
> >> >
> >> > > How come this patch is not included in the mainline source?
> >> >
> >> > Well, partly because you're the first person to confirm that it works
> >> > for someone besides me :). Dan (Cross) has offered to test it out as
> >> > well; if I can get a couple of confirmations that it's working, I'll
> >> > probably go ahead and submit it to netdev.
> >>
> >> I am running this as of today, and so far so good.
> >>
> >> > The bigger issue has been that there are very few people interested in
> >> > the ax.25 support in the kernel; there's not a real maintainer, so it's
> >> > difficult to get code approved. There were some concerns expressed that
> >> > maybe this isn't the *correct* fix, but I would argue that even if it's
> >> > not the most correct fix we should try to get it in anyway, since
> >> > otherwise ax.25 is completely broken.
> >>
> >> Agreed: what's there now is objectively broken. Please, let's not let
> >> the perfect be the enemy of the good here.
> >>
> >>         - Dan C.
> >
> >
> >
> > --
> > Thanks,
> > Chris Maness



-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Mike Quin]

> On 21 May 2024, at 13:03, Chris Maness <christopher.maness@gmail.com> wrote:
> 
> netrom_call has not worked for ages.  I am using VE7FET's forked
> userspace, not the official.  I could build another
> system with the official userspace I suppose.  My understanding is
> that VE7FET fixed some bugs, and I have had
> better luck with it for the most part.

I’ve found axcall (as opposed to ax25_call, netrom_call etc) as shipped by Debian works fine for making netrom connections.

To echo other comments in the thread, thanks to the folks working on this. I’m one of a number of amateurs who’ve taken a recent interest in packet and I’ve love to make more use of Linux for it.

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

My test bed is Slackware-current because it has the latest kernel
source built right in.  So I have to use either VE7FET's userspace, or
the stock Linux-hams stuff as Slackware does not have a repository per
se.  It is just all lumped together with no package dependency
checking.  This is good and bad for somethings, but turns out to make
it really easy to test sock code -- because it is all just the way it
was written with very little system specific patches.  The kernel code
has absolutely no distro specific modifications.

So whatever aps come ax25-tools and ax25-apps is all I have.

-Chris KQ6UP

On Tue, May 21, 2024 at 12:31 PM Mike Quin <mike@elite.uk.com> wrote:
>
>
>
> > On 21 May 2024, at 13:03, Chris Maness <christopher.maness@gmail.com> wrote:
> >
> > netrom_call has not worked for ages.  I am using VE7FET's forked
> > userspace, not the official.  I could build another
> > system with the official userspace I suppose.  My understanding is
> > that VE7FET fixed some bugs, and I have had
> > better luck with it for the most part.
>
> I’ve found axcall (as opposed to ax25_call, netrom_call etc) as shipped by Debian works fine for making netrom connections.
>
> To echo other comments in the thread, thanks to the folks working on this. I’m one of a number of amateurs who’ve taken a recent interest in packet and I’ve love to make more use of Linux for it.
>
>


-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Mike Quin]

> On 22 May 2024, at 00:02, Chris Maness <christopher.maness@gmail.com> wrote:
> 
> My test bed is Slackware-current because it has the latest kernel
> source built right in.  So I have to use either VE7FET's userspace, or
> the stock Linux-hams stuff as Slackware does not have a repository per
> se.  It is just all lumped together with no package dependency
> checking.  This is good and bad for somethings, but turns out to make
> it really easy to test sock code -- because it is all just the way it
> was written with very little system specific patches.  The kernel code
> has absolutely no distro specific modifications.
> 
> So whatever aps come ax25-tools and ax25-apps is all I have.

Looks like it’s just called ‘call’ int the original ax25-apps.

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

Ah, I was not aware that it did NETROM connects too.  I have only used
it for vanilla AX.25.  And yes it works, so the issue must be in
UROnode.

-Chris KQ6UP

On Tue, May 21, 2024 at 4:07 PM Mike Quin <mike@elite.uk.com> wrote:
>
>
>
> > On 22 May 2024, at 00:02, Chris Maness <christopher.maness@gmail.com> wrote:
> >
> > My test bed is Slackware-current because it has the latest kernel
> > source built right in.  So I have to use either VE7FET's userspace, or
> > the stock Linux-hams stuff as Slackware does not have a repository per
> > se.  It is just all lumped together with no package dependency
> > checking.  This is good and bad for somethings, but turns out to make
> > it really easy to test sock code -- because it is all just the way it
> > was written with very little system specific patches.  The kernel code
> > has absolutely no distro specific modifications.
> >
> > So whatever aps come ax25-tools and ax25-apps is all I have.
>
> Looks like it’s just called ‘call’ int the original ax25-apps.
>


-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[David Ranch]

Hello Lars,

> Well, partly because you're the first person to confirm that it works
> for someone besides me :). Dan (Cross) has offered to test it out as
> well; if I can get a couple of confirmations that it's working, I'll
> probably go ahead and submit it to netdev.
>
> The bigger issue has been that there are very few people interested in
> the ax.25 support in the kernel; there's not a real maintainer, so it's
> difficult to get code approved. There were some concerns expressed that
> maybe this isn't the *correct* fix, but I would argue that even if it's
> not the most correct fix we should try to get it in anyway, since
> otherwise ax.25 is completely broken.

First off, Lars, I wanted to thank you for getting some of these REAL 
fixes actually committed into the mainline.  You've done something that 
many others could not!  That said, I wanted to chime in here about your 
commend about "very few people interested in the a.25 support in the 
kernel".  That's definitely not true.

I and several others tried for many quarters and years to try to get 
some of these initial toxic commits that came in around the 4.19.x 
timeframe fixed but there was never any real meaningful responses and 
fixes.  I do agree that the lack of a real maintainer aka steward for 
the mkiss / ax25 / netrom / rose kernel code really hurts the ongoing 
health of the stack.  I wish I could take up that job but I don't have 
the required skillset.  The larger issue here is that random Linux 
kernel developers are periodically changing various parts of kernel 
infrastructure and when they try to make "updates" to the AX.25 code, 
the mindset is that they only consider a successful kernel compile as a 
"code pass".  That's VERY bad.  The other significant issue is that the 
kernel branch maintainers / powers at be seem to just accept these 
changes without any real scrutiny and don't require any real testing 
results to confirm things still work.  I had previously asked how some 
of these developers had really tested their code when they offered 
official patches here on the vger list and never received *any* 
meaningful responses.  I've asked if there is some form of a per-commit 
or daily CI build and regression test environment that I could offer up 
some basic toxicity test scenarios to.  No response.  It's been hugely 
frustrating for YEARS now and all of the "silently suffering" Linux 
packet users have resorted to still run ancient Linux distros like 
Debian Wheezy to keep their systems fully operational.

I am hugely grateful to you (Lars) on being able to submit some 
meaningful fixes and was able to get them committed to the mainline.  
The next major hurdle that I would love to get feedback on is how to get 
these new changes but also create a set of backported fixes committed 
into various Linux distributions kernels that don't strictly follow the 
mainline kernel.  My personal interest is in the Raspberry Pi OS kernel 
and the Canonical Ubuntu 22.04 / 20.04 kernels.  If people have 
recommendations / contacts to try / etc...please contact me offline and 
I will work on trying to get these fixes committed.

--David
KI6ZHD
Maintainer of the Linpac AX.25 terminal program
Advocate of the Direwolf multi-platform AX.25 software based TNC
AMPR 44-net Coordinator for the Silicon Valley, USA region

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

I had some patches committed to the Raspberry Pi fork, and that was
just a matter of submitting a bug report in their forked github kernel
site.  It was easy since the kernel had already been committed in the
mainline, they just simply added the patches the way I did before
recompiling and installing it on my Raspberry Pi.  I wish I could help
more.  I have not done any programming beyond what I needed to do to
get my physics degree.

-Chris KQ6UP

On Tue, May 21, 2024 at 8:53 AM David Ranch <linux-hams@trinnet.net> wrote:
>
>
> Hello Lars,
>
> > Well, partly because you're the first person to confirm that it works
> > for someone besides me :). Dan (Cross) has offered to test it out as
> > well; if I can get a couple of confirmations that it's working, I'll
> > probably go ahead and submit it to netdev.
> >
> > The bigger issue has been that there are very few people interested in
> > the ax.25 support in the kernel; there's not a real maintainer, so it's
> > difficult to get code approved. There were some concerns expressed that
> > maybe this isn't the *correct* fix, but I would argue that even if it's
> > not the most correct fix we should try to get it in anyway, since
> > otherwise ax.25 is completely broken.
>
> First off, Lars, I wanted to thank you for getting some of these REAL
> fixes actually committed into the mainline.  You've done something that
> many others could not!  That said, I wanted to chime in here about your
> commend about "very few people interested in the a.25 support in the
> kernel".  That's definitely not true.
>
> I and several others tried for many quarters and years to try to get
> some of these initial toxic commits that came in around the 4.19.x
> timeframe fixed but there was never any real meaningful responses and
> fixes.  I do agree that the lack of a real maintainer aka steward for
> the mkiss / ax25 / netrom / rose kernel code really hurts the ongoing
> health of the stack.  I wish I could take up that job but I don't have
> the required skillset.  The larger issue here is that random Linux
> kernel developers are periodically changing various parts of kernel
> infrastructure and when they try to make "updates" to the AX.25 code,
> the mindset is that they only consider a successful kernel compile as a
> "code pass".  That's VERY bad.  The other significant issue is that the
> kernel branch maintainers / powers at be seem to just accept these
> changes without any real scrutiny and don't require any real testing
> results to confirm things still work.  I had previously asked how some
> of these developers had really tested their code when they offered
> official patches here on the vger list and never received *any*
> meaningful responses.  I've asked if there is some form of a per-commit
> or daily CI build and regression test environment that I could offer up
> some basic toxicity test scenarios to.  No response.  It's been hugely
> frustrating for YEARS now and all of the "silently suffering" Linux
> packet users have resorted to still run ancient Linux distros like
> Debian Wheezy to keep their systems fully operational.
>
> I am hugely grateful to you (Lars) on being able to submit some
> meaningful fixes and was able to get them committed to the mainline.
> The next major hurdle that I would love to get feedback on is how to get
> these new changes but also create a set of backported fixes committed
> into various Linux distributions kernels that don't strictly follow the
> mainline kernel.  My personal interest is in the Raspberry Pi OS kernel
> and the Canonical Ubuntu 22.04 / 20.04 kernels.  If people have
> recommendations / contacts to try / etc...please contact me offline and
> I will work on trying to get these fixes committed.
>
> --David
> KI6ZHD
> Maintainer of the Linpac AX.25 terminal program
> Advocate of the Direwolf multi-platform AX.25 software based TNC
> AMPR 44-net Coordinator for the Silicon Valley, USA region
>


-- 
Thanks,
Chris Maness

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Tue, May 21, 2024 at 08:53:06AM GMT, David Ranch wrote:
> That said, I wanted to chime in here about your commend
> about "very few people interested in the a.25 support in the kernel".
> That's definitely not true.

That's good to know! I based that on the amount of traffic I see here on
the linux-hams list, and the difficulty in getting anybody to offer an
opinion (or an alternative to) these changes. If there is a more active
forum where people are talking about Linux and amateur radio I'm happy
to join there as well.

> The other significant issue is that the kernel branch maintainers /
> powers at be seem to just accept these changes without any real
> scrutiny and don't require any real testing results to confirm things
> still work. 

I think this would typically be the job of a subsystem maintainer; this
is another way in which the lack of a maintainer hurts the
ax.25 stack.

> I've asked if there is some form of a per-commit or daily CI
> build and regression test environment that I could offer up some basic
> toxicity test scenarios to.

Have you put together an ax.25 test suite? I put together an automated
test for the particular problem I'm trying to resolve (that was
necessary in order to successfully use `git bisect run`); collecting
these sorts of things in one place would be a great step forward.

> I am hugely grateful to you (Lars) on being able to submit some meaningful
> fixes and was able to get them committed to the mainline.

Just to clear, that hasn't happened yet! The patch needs to get
approved first.

> The next major hurdle that I would love to get feedback on is how to
> get these new changes but also create a set of backported fixes
> committed into various Linux distributions kernels that don't strictly
> follow the mainline kernel.  My personal interest is in the Raspberry
> Pi OS kernel...

I'm running this patch on kernel 6.6.30 on my Raspberry Pi systems; you
can find the patch against 6.6.30 here:

  https://github.com/larsks/ax25-debugging/tree/main/patches-for-6.6.30

It's made my Pis much more stable. I think that if the patch gets
accepted for the mainline kernel it should be relatively easy to get
into the Raspberry Pi kernel as well.

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

Re: Kernel 6.9.1 AX.25 Crash

[Dan Cross]

On Tue, May 21, 2024 at 2:51 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
> On Tue, May 21, 2024 at 08:53:06AM GMT, David Ranch wrote:
> > That said, I wanted to chime in here about your commend
> > about "very few people interested in the a.25 support in the kernel".
> > That's definitely not true.
>
> That's good to know! I based that on the amount of traffic I see here on
> the linux-hams list, and the difficulty in getting anybody to offer an
> opinion (or an alternative to) these changes. If there is a more active
> forum where people are talking about Linux and amateur radio I'm happy
> to join there as well.
>
> > The other significant issue is that the kernel branch maintainers /
> > powers at be seem to just accept these changes without any real
> > scrutiny and don't require any real testing results to confirm things
> > still work.
>
> I think this would typically be the job of a subsystem maintainer; this
> is another way in which the lack of a maintainer hurts the
> ax.25 stack.
> [snip]

I mentioned this to Lars privately a few days ago, but wanted to share
more broadly, hopefully to foster some discussion about the future of
packet and AX.25 on Linux (and similar!) platforms generally.
Hopefully Lars's patch gets integrated into mainline soon, but I can't
help feeling we've got a lot more work ahead to bring the stack up to
a sufficiently high quality standard.

My personal belief is that the best way forward is to rip AX.25,
NETROM, and Rose out of the kernel and go with a pure user-space
implementation. I can't really think of any use cases that benefit
from being in-kernel; use cases like IP-over-AX.25 could be handled
using TUN/TAP, for example (and I think that's probably the most
obvious one where kernel integration is useful). At the speeds we're
talking about here, crossing the user<->kernel boundary isn't a
significant performance hit.

But I can think of many drawbacks, some of which have been mentioned
in this thread. The lack of a dedicated maintainer is a serious issue,
and it's clear that AX.25 has atrophied significantly over the years.
Moreover, the userspace tools have bitrotted to some extent, and lack
clear maintainers. And tools like `ss` have not implemented support
for AX.25 or netrom: we are already behind the curve. The relatively
slow pace required to get things into the kernel, let alone filter
down to individual distributions, is another.

On the other hand, with a pure userspace implementation, AX.25 support
becomes a normal program like any other, and can improve at its own
pace, not tied to the kernel release schedules of any particular
distribution. As I mentioned, IP-over-AX.25 can be supported using
TUN/TAP; existing programs that expect to use the sockets interface
could be supported via a compatibility library and the `DL_PRELOAD`
trick. Presumably the actual implementation would expose a
well-defined interface over a local IPC channel (e.g., RPCs over a
Unix domain socket, or something like that) so that new clients could
be written in a variety of languages (I wouldn't mind Rust, or Go; I'm
sure others would love Python), without necessarily having to write
bindings for the sockets-specific bits.

A way to start here might be a multistage process of extraction from the kernel:
1. Copy the existing code into some directory.
2. Work on getting it to compile, but not link.
3. Bundle the resulting objects up into a library.
4. Start writing shim functions for the missing bits required to link;
plug this into a different library.
    Once there's a minimally linking binary (even if it doesn't do
anything), you've got a pretty good idea of the requirements the stack
needs just to be buildable; this also naturally exposes a portability
interface.
5. Flesh those stubs into a real implementation, implementing missing
components and a daemon (or two or three or whatever) to run it in.
6. Bask, flush in the face of success.

Of course, there are existing userspace AX.25 implementations: BPQ et
al come to mind. Thomas Habets started work on a userspace
implementation (https://blog.habets.se/2021/11/AX25-user-space.html,
but that seems to have stalled). But having a AX.25 available as a
generalized library and set of components, usable in the same manner
that the existing kernel stack is, would be a win for building new,
novel applications.

Thoughts?

        - Dan C. (KZ2X)

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

> My personal belief is that the best way forward is to rip AX.25,
> NETROM, and Rose out of the kernel and go with a pure user-space
> implementation. I can't really think of any use cases that benefit
> from being in-kernel; use cases like IP-over-AX.25 could be handled
> using TUN/TAP, for example (and I think that's probably the most
> obvious one where kernel integration is useful). At the speeds we're
> talking about here, crossing the user<->kernel boundary isn't a
> significant performance hit.
>

I think in doing this you will orphan a lot of important software that
relies on the kernel space ax.25 stack.  A prime example would be
LinFBB.  LinFBB requires either an ax0 or a hostmode TNC.  Hostmode
TNC's are getting to be really long in the tooth by this point.  There
would be no way to implement direwolf as a radio port in that case as
FBB does not do KISS.  You would also lose netrom capabilities of this
BBS.  I feel that FBB has the best BBS user experience, BPQ32 copied
it, but FBB is still better in my opinion.  It also has features that
BPQ32 does not have.  A possible workaround is to revive NORD><LINK's
THENETNODE project that went to mothballs in 2006.  I am not sure on
the licensing, but the source code is still available.  TFKISS can
then emulate a hostmode TNC while TNN provides NETROM, KISS, AX.25,
INP, and Flexnet.  I think if that code was brought up to date, it
would be a good candidate to replace the kernel space code.  I have
not been able to get anyone to take me up on this project.  I am not a
programmer, or I would work on it myself.

There are other softwares out there that there is no full replacement
for.  linpac would be another example.  I think the bottom line is a
massive drop in the interest in packet radio as a whole since yr.
2000.  There is not the userbase of packet radio in its heyday, and we
have far passed the high water mark.  There is some pretty amazing
code out there that was just left to languish.  If you have ever had a
chance to play with WinFBB, you would know.  That is such a refined
BBS with its own built in email editor.  Pretty neat setup, but
development stopped in 2001.  Many of the important German
contributors don't even seem to be hams anymore.

I vote leave it in (even if it is limping along) because this last
patch seems to put my system back into a usable state as far as LinFBB
and NETROM, if you rip it out, I am back to using ancient distros.
Slackware 14.1 (Kernel version 3) for a rock stable AX.25 setup.

Those are my $.02

-Chris KQ6UP

Re: Kernel 6.9.1 AX.25 Crash

[David Ranch]

Hello Everyone,

> I think in doing this you will orphan a lot of important software that
> relies on the kernel space ax.25 stack.

I agree with Dan's idea that moving the Linux AX.25 stack out from 
kernel space could help on the maintenance side but it would absolutely 
neuter things when it comes to all the rich packet routing options that 
the kernel offers today.

One thing that could be considered is an alternative libax25 library 
that could redirect ax25/netrom/rose packet I/O from going to the kernel 
and instead, send that traffic to a userland program.  Maybe that could 
be Direwolf or QtSoundmodem, etc.  I think this could work for most of 
standard (more simplistic) packet radio use cases.  I've mentioned this 
idea some time ago to both Thomas Osterried DL9SAU who is the maintainer 
of the "Official" AX.25 userland tools as well as Lee Woldanski VE7FET 
who maintains a fork of the AX.25 userland tools but there never has 
been much interest.

Maybe someone here on this list might be interested in taking a stab at 
it?  I don't have the programming chops to write the code but I could 
absolutely help on many other aspects (integration, testing, etc) if needed.

--David
KI6ZHD

Re: Kernel 6.9.1 AX.25 Crash

[Dan Cross]

On Wed, May 22, 2024 at 1:06 PM David Ranch <linux-hams@trinnet.net> wrote:
> > I think in doing this you will orphan a lot of important software that
> > relies on the kernel space ax.25 stack.
>
> I agree with Dan's idea that moving the Linux AX.25 stack out from
> kernel space could help on the maintenance side but it would absolutely
> neuter things when it comes to all the rich packet routing options that
> the kernel offers today.

I wonder why, and in what way? It's not clear to me that the kernel
has any particular advantage for routing AX.25/netrom/rose packets
over a userspace program. What does it currently do that could not be
duplicated, or would be exceptionally difficult to duplicate,
elsewhere?

Indeed, I would think that a packet router of some kind would be an
essential feature.

> One thing that could be considered is an alternative libax25 library
> that could redirect ax25/netrom/rose packet I/O from going to the kernel
> and instead, send that traffic to a userland program.  Maybe that could
> be Direwolf or QtSoundmodem, etc.  I think this could work for most of
> standard (more simplistic) packet radio use cases.  I've mentioned this
> idea some time ago to both Thomas Osterried DL9SAU who is the maintainer
> of the "Official" AX.25 userland tools as well as Lee Woldanski VE7FET
> who maintains a fork of the AX.25 userland tools but there never has
> been much interest.
>
> Maybe someone here on this list might be interested in taking a stab at
> it?  I don't have the programming chops to write the code but I could
> absolutely help on many other aspects (integration, testing, etc) if needed.
>
> --David
> KI6ZHD

Re: Kernel 6.9.1 AX.25 Crash

[Dan Cross]

On Wed, May 22, 2024 at 12:07 PM Chris Maness
<christopher.maness@gmail.com> wrote:
> > My personal belief is that the best way forward is to rip AX.25,
> > NETROM, and Rose out of the kernel and go with a pure user-space
> > implementation. I can't really think of any use cases that benefit
> > from being in-kernel; use cases like IP-over-AX.25 could be handled
> > using TUN/TAP, for example (and I think that's probably the most
> > obvious one where kernel integration is useful). At the speeds we're
> > talking about here, crossing the user<->kernel boundary isn't a
> > significant performance hit.
>
> I think in doing this you will orphan a lot of important software that
> relies on the kernel space ax.25 stack.  A prime example would be
> LinFBB.  LinFBB requires either an ax0 or a hostmode TNC.

I'm not entirely sure why.  All of those programs that currently
expect the kernel stack are userspace programs, and all of them
interact with the stack via a set of interface functions that are
provided in the C library; of course, those functions are implemented
in terms of the kernel system call interface, but to a first order
approximation, practically nothing talks to the kernel _directly_;
they all use a(n albeit thin) shim interface. Thus, if a compatibility
library were created that implemented those interfaces, and those
programs arranged to link that library dynamically when they started
up, they could continue to operate unchanged. Of course they would no
longer be using the kernel stack, but they'd neither know (nor, I
argue, care). This is the essence of "the `LD_PRELOAD` trick" I
mentioned earlier.

> Hostmode
> TNC's are getting to be really long in the tooth by this point.  There
> would be no way to implement direwolf as a radio port in that case as
> FBB does not do KISS.  You would also lose netrom capabilities of this
> BBS.  I feel that FBB has the best BBS user experience, BPQ32 copied
> it, but FBB is still better in my opinion.  It also has features that
> BPQ32 does not have.  A possible workaround is to revive NORD><LINK's
> THENETNODE project that went to mothballs in 2006.  I am not sure on
> the licensing, but the source code is still available.  TFKISS can
> then emulate a hostmode TNC while TNN provides NETROM, KISS, AX.25,
> INP, and Flexnet.  I think if that code was brought up to date, it
> would be a good candidate to replace the kernel space code.  I have
> not been able to get anyone to take me up on this project.  I am not a
> programmer, or I would work on it myself.
>
> There are other softwares out there that there is no full replacement
> for.  linpac would be another example.  I think the bottom line is a
> massive drop in the interest in packet radio as a whole since yr.
> 2000.  There is not the userbase of packet radio in its heyday, and we
> have far passed the high water mark.  There is some pretty amazing
> code out there that was just left to languish.  If you have ever had a
> chance to play with WinFBB, you would know.  That is such a refined
> BBS with its own built in email editor.  Pretty neat setup, but
> development stopped in 2001.  Many of the important German
> contributors don't even seem to be hams anymore.
>
> I vote leave it in (even if it is limping along) because this last
> patch seems to put my system back into a usable state as far as LinFBB
> and NETROM, if you rip it out, I am back to using ancient distros.
> Slackware 14.1 (Kernel version 3) for a rock stable AX.25 setup.
>
> Those are my $.02

I think it's important to explain that existing software wouldn't a
priori stop working; indeed, a central goal of the effort would be to
maintain compatibility! This is easily doable these days.

        - Dan C.

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Wed, May 22, 2024 at 11:32:21AM GMT, Dan Cross wrote:
> My personal belief is that the best way forward is to rip AX.25,
> NETROM, and Rose out of the kernel and go with a pure user-space
> implementation.

I'm generally in favor of this idea, as long as we sequence things so
that we have a stable, functioning userspace before ax.25 support is
dropped from the kernel (and that we're able to maintain compatability
-- as much as possible -- with the existing tools).

Dan, are you volunteering to spearhead this effort? I'm happy to help
out, but I don't think I should be driving; I don't have enough
familiarity with the protocols involved.

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

Re: Kernel 6.9.1 AX.25 Crash

[Dan Cross]

On Wed, May 22, 2024 at 2:17 PM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
> On Wed, May 22, 2024 at 11:32:21AM GMT, Dan Cross wrote:
> > My personal belief is that the best way forward is to rip AX.25,
> > NETROM, and Rose out of the kernel and go with a pure user-space
> > implementation.
>
> I'm generally in favor of this idea, as long as we sequence things so
> that we have a stable, functioning userspace before ax.25 support is
> dropped from the kernel (and that we're able to maintain compatability
> -- as much as possible -- with the existing tools).

Absolutely. It may be the case that the stack is never actually
removed from the kernel, or is removed at some point far in the
future. In any event, I'd hoped it would go without saying that
nothing would be proposed for removal until there was an alternative
that worked at least as well readily available.

Again, I want to stress: nothing would be taken away, either in terms
of functionality or compatibility with existing programs. But there's
nothing magical about AX.25 being in the kernel; if anything, it's far
more constrained as a result.

> Dan, are you volunteering to spearhead this effort? I'm happy to help
> out, but I don't think I should be driving; I don't have enough
> familiarity with the protocols involved.

I'm happy to take a swing at starting to see what's involved from
(metaphorically) pulling the engine from the car and getting it
sitting on a stand with a bunch of tubes and wires hanging out. I'll
be blunt: my time is limited, but this _is_ something I'd like to see
happen. Maybe the best way to proceed is an informal PoC that amounts
to, "look, here's a git repo and a Makefile that'll generate a bunch
of unlinkable object files, but compiled entirely out of tree and in
userspace."

        - Dan C.

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Wed, May 22, 2024 at 02:37:06PM GMT, Dan Cross wrote:
> Maybe the best way to proceed is an informal PoC that amounts
> to, "look, here's a git repo and a Makefile that'll generate a bunch
> of unlinkable object files, but compiled entirely out of tree and in
> userspace."

+1 :).

Wondering out loud: Direwolf already has a user-space ax.25 implementation
(and apparently IL2P); would that be a better starting point than the
kernel implementation?

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

Re: Kernel 6.9.1 AX.25 Crash

[David Ranch]

I've already asked several people about this and the reality is that the 
Direwolf AX.25 protocol stack is currently only accessible via the AGW 
interface.  I then asked if the Linux libax25 user-space library could 
be adapted to make AGW calls and the answer I've received was: partially.

There are a lot of fine protocol details in the Linux stack that are NOT 
exposed via the "defacto" AGW API standard that stands today.  Maybe if 
the AGW API was heavily enhanced, maybe it would be possible but there 
is a whole other level of potential legal issues with that.
The following was posted years after tools like ldsped, soundmodem, 
direwolf, etc. were already creating AGW "Server" API services:

https://www.sv2agw.com/Home/Developers
--
License Agreement

You must not reverse engineering the TCP/IP protocol that Packet Engine 
uses to communicate with client applications. This protocol copyright 
belongs to me and you cannot emulate it. You can only use it for writing 
client applications.

Your program is totally independent from Packet Engine. You can disturb 
your program any way you like. Freeware, Shareware or as commercial 
application. Since AGW Packet Engine is self-standing application its 
license agreement is not applied to your program. However the end user 
must respect the Packet Engine License agreement.
--

--David
KI6ZHD


>
> Wondering out loud: Direwolf already has a user-space ax.25 implementation
> (and apparently IL2P); would that be a better starting point than the
> kernel implementation?
>

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Wed, May 22, 2024 at 03:22:05PM GMT, David Ranch wrote:
> I've already asked several people about this and the reality is that the
> Direwolf AX.25 protocol stack is currently only accessible via the AGW
> interface.  I then asked if the Linux libax25 user-space library could be
> adapted to make AGW calls and the answer I've received was: partially.

I wasn't really suggesting that we integrate with Direwolf; I was
suggesting that if we were going to rip an existing AX.25 implementation
out of something in order to create a general purpose user space
implementation, maybe the Direwolf code would be a better choice than
the kernel code (since it has been much more actively maintained).

We'd end up with something like "libdirewolf".

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

Userspace AX.25 stack library [was Re: Kernel 6.9.1 AX.25 Crash]

[Stuart Longland VK4MSL]

On 23/5/24 08:22, David Ranch wrote:
> There are a lot of fine protocol details in the Linux stack that are NOT 
> exposed via the "defacto" AGW API standard that stands today.  Maybe if 
> the AGW API was heavily enhanced, maybe it would be possible but there 
> is a whole other level of potential legal issues with that.
> The following was posted years after tools like ldsped, soundmodem, 
> direwolf, etc. were already creating AGW "Server" API services:
> 
> https://www.sv2agw.com/Home/Developers
> -- 
> License Agreement
> 
> You must not reverse engineering the TCP/IP protocol that Packet Engine 
> uses to communicate with client applications. This protocol copyright 
> belongs to me and you cannot emulate it. You can only use it for writing 
> client applications.

Ehh, fun stuff.  I suppose the question then is, what would a home-grown 
packet API look like?

I recall looking at the AGW API and observing some of its limitations 
(the full list illudes me just now, but I recall things like the 
reserved bits were inaccessible).

One thing I observed recently, is that other userspace stacks like BPQ32 
will switch protocols after connecting, so the initial I-frame might be 
sent without routing protocol data (PID 0xf0), but then it'll switch to 
NETROM (PID 0xcf) with its own "frame" format within.

https://gist.github.com/sjlongland/0199d5a7cc7bbc71e83d0cc5577a2509

For someone like myself who's been used to dealing with TCP/IP, AX.25 
seems quite weird… yes, there's UI frames that behave like UDP and 
connected-mode that's sort-of like TCP; but then that connected-mode 
stream is actually functions as a multiplex separated by PID code.

I haven't tried picking apart NETROM yet, nor have I looked at how ROSE 
was structured.  Finding relevant documentation for these protocols is 
getting tricky now.  So the Linux kernel stack, for all its faults and 
failings, is still valuable as it at least implements these once 
documented protocols.

Do we start with maybe defining a wire-representation of the Linux 
kernel AX.25 stack data types and an RPC mechanism?  Or should we toss 
that and start afresh?

Regards,
-- 
Stuart Longland (aka Redhatter, VK4MSL)

I haven't lost my mind...
   ...it's backed up on a tape somewhere.

Re: Userspace AX.25 stack library [was Re: Kernel 6.9.1 AX.25 Crash]

[Dan Cross]

On Wed, May 22, 2024 at 8:55 PM Stuart Longland VK4MSL <me@vk4msl.com> wrote:
> On 23/5/24 08:22, David Ranch wrote:
> > There are a lot of fine protocol details in the Linux stack that are NOT
> > exposed via the "defacto" AGW API standard that stands today.  Maybe if
> > the AGW API was heavily enhanced, maybe it would be possible but there
> > is a whole other level of potential legal issues with that.
> > The following was posted years after tools like ldsped, soundmodem,
> > direwolf, etc. were already creating AGW "Server" API services:
> >
> > https://www.sv2agw.com/Home/Developers
> > --
> > License Agreement
> >
> > You must not reverse engineering the TCP/IP protocol that Packet Engine
> > uses to communicate with client applications. This protocol copyright
> > belongs to me and you cannot emulate it. You can only use it for writing
> > client applications.
>
> Ehh, fun stuff.

Yikes. That's scary looking.

> I suppose the question then is, what would a home-grown
> packet API look like?

To spitball around this for a moment, it strikes me that there are
several different aspects one must account for. AX.25 is a link-layer
protocol, so it has to communicate with a physical layer below it, and
a network layer above it. Kinda sorta; it gets a little funky because
it's based on X.25 which predated a clean model for layered
separation, and further came out of the telco world where things were
circuit switched for actual communication, with out-of-band channels
for control signalling. But to a first order approximation, this is
accurate. So we've got two use cases:

1. AX.25<->physical layer

There's the interface between the host and the physical layer (RF path
or AXUDP or AXIP or whatever). Lots of folks are using KISS TNCs for
this, either real hardware devices or something like Direwolf. But
that's the easy part; you've just got to have an interface that lets
you send and receive frames, with error detection. KISS would be fine.
AGW would probably be ok, here; for instance `paracon` does this
(https://pyham.org/en/latest/#paracon).

2. AX.25<->network layer (aka, application software)

This is where it gets a little bit more complicated. For the sake of
argument, assume there exists an "protocol, interface and routing"
daemon that implemented AX.25; the daemon's job is to present a set of
virtual interfaces, analogous to the existing ax* interfaces in the
Linux kernel stack. It is also in charge of managing AX.25
connections, keeping track of them, associating them with interfaces,
and routing between them. Basically, it does what the kernel stack
does today.

Now the question becomes, how do application programs interact with
that daemon? Well, there are a couple of different bits of
functionality that need to be exposed: there are administrative
interfaces for setting global and per-interface configuration
parameters, for querying those parameters, and for querying the state
of connections, routes, and so forth. Then there's got to be a means
of actually moving data around.

If we look at how all of these things are implemented today, it's via
system calls; the administrative stuff is mostly via `ioctl`s on a
socket descriptor. Data itself is moved around using the sockets
interface; e.g., creating a socket using `socket(AF_AX25, ..., ...)`
(or `AF_NETROM` or whatever). Then you can establish connections via
connect/bind/listen/accept (or not, if using datagram sockets and the
*msg functions). One then transfers data over that socket using the
normal sockets system calls:
read/write/send/recv/sendto/recvfrom/sendmsg/recvmsg and their
variants, etc. The socket options and name interfaces (e.g.,
`getpeername`) are available.

Of course, we don't invoke those syscalls directly, but rather, we do
so by calling stub functions in the C library that invoke them on our
behalf (e.g., on x86, poking the system call number into %rax, and
executing the `SYSCALL` instruction).

Anyway, my suggestion would be to replace those syscalls with RPCs.
Using something like gRPC, we could create a service definition
comprised RPCs corresponding to each call, where the request data
mirrored the system call's arguments. The response data would, in
turn, mirror the system call's return values and so forth. The various
data currently exposed by the sockets API could be represented as
individual protobuf message types. Of course, it need not be as
completely general as the actual sockets API (no need to support the
sockaddr_in* types, for example; just sockaddr_{ax25,netrom,rose}).

So to an unnumbered AX.25 information frame, for example, one might
expose a "sendmsg" RPC that takes a request structure with a interface
identifier, source and destination call signs, any relevant flags, and
a data vector; a call to that would cause a handler in the daemon to
extract the message data, construct an AX.25 frame, and queue that to
the physical layer associated with the device.

A client compatibility library that implements the sockets interface,
but translates to gRPC calls behind the scenes, could be implemented
and used by existing clients so that they bypass the kernel stack.

> I recall looking at the AGW API and observing some of its limitations
> (the full list illudes me just now, but I recall things like the
> reserved bits were inaccessible).
>
> One thing I observed recently, is that other userspace stacks like BPQ32
> will switch protocols after connecting, so the initial I-frame might be
> sent without routing protocol data (PID 0xf0), but then it'll switch to
> NETROM (PID 0xcf) with its own "frame" format within.
>
> https://gist.github.com/sjlongland/0199d5a7cc7bbc71e83d0cc5577a2509
>
> For someone like myself who's been used to dealing with TCP/IP, AX.25
> seems quite weird… yes, there's UI frames that behave like UDP and
> connected-mode that's sort-of like TCP; but then that connected-mode
> stream is actually functions as a multiplex separated by PID code.

I don't find this too terribly surprising, given the history of the
protocol and its origins in X.25. I guess I tend to think of UI frames
as being more like Ethernet frames, and connected-mode AX.25 more like
frame-relay PVCs.

> I haven't tried picking apart NETROM yet, nor have I looked at how ROSE
> was structured.  Finding relevant documentation for these protocols is
> getting tricky now.  So the Linux kernel stack, for all its faults and
> failings, is still valuable as it at least implements these once
> documented protocols.

+1!

> Do we start with maybe defining a wire-representation of the Linux
> kernel AX.25 stack data types and an RPC mechanism?  Or should we toss
> that and start afresh?

I think this makes the most sense; personally, I'd start with protobuf
and gRPC, but that's me.

        - Dan C.

Re: Kernel 6.9.1 AX.25 Crash

[David Ranch]

Hello Lars,

> That's good to know! I based that on the amount of traffic I see here on
> the linux-hams list, and the difficulty in getting anybody to offer an
> opinion (or an alternative to) these changes. If there is a more active
> forum where people are talking about Linux and amateur radio I'm happy
> to join there as well.

Just to name a few, consider joining:

    Direwolf software-tnc:  https://groups.io/g/direwolf

    Raspberry Pi HAM Radio: https://groups.io/g/RaspberryPi-4-HamRadio


> I think this would typically be the job of a subsystem maintainer; this
> is another way in which the lack of a maintainer hurts the
> ax.25 stack.

But since there isn't one.. why are these broken commits still being 
allowed?


> Have you put together an ax.25 test suite? I put together an automated
> test for the particular problem I'm trying to resolve (that was
> necessary in order to successfully use `git bisect run`); collecting
> these sorts of things in one place would be a great step forward.

I've offered to do so but I never received any response.  I need to know 
what is needed here:

    - Just document various testcase scenarios?

    - Create automation to run the test cases?  In what language?  Bash? 
  Python?  Perl?  JavaScript?

    - Create a set of pre-configured inter-connected VM images for 
running automated tests?  If so, what format, what CPU architecture, etc


>> I am hugely grateful to you (Lars) on being able to submit some meaningful
>> fixes and was able to get them committed to the mainline.
>
> Just to clear, that hasn't happened yet! The patch needs to get
> approved first.

Sigh... ok.


>> The next major hurdle that I would love to get feedback on is how to
>> get these new changes but also create a set of backported fixes
>> committed into various Linux distributions kernels that don't strictly
>> follow the mainline kernel.  My personal interest is in the Raspberry
>> Pi OS kernel

My primary focus is on Raspberry Pi as well but also X86 computers too.


> I'm running this patch on kernel 6.6.30 on my Raspberry Pi systems; you
> can find the patch against 6.6.30 here:
>
>   https://github.com/larsks/ax25-debugging/tree/main/patches-for-6.6.30
>
> It's made my Pis much more stable. I think that if the patch gets
> accepted for the mainline kernel it should be relatively easy to get
> into the Raspberry Pi kernel as well.

Are you patching and compiling the Raspberry Pi OS specific kernel tree 
or are you using the mainline Linux-Next or primary Linus tree?

--David
KI6ZHD

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Wed, May 22, 2024 at 09:10:11AM GMT, David Ranch wrote:
> Just to name a few, consider joining:

Thanks! 

> I've offered to do so but I never received any response.  I need to know
> what is needed here:

I Am Not The Maintainer :), but I think my preference would be to start
with:

>    - Just document various testcase scenarios?

And then, or at the same time, create the necessary automation to run
these scenarios. Depending on the complexity of the tasks I would start
with Bash scripts, but would probably move to Python (in preference to
Perl or Javascript) if we needed something more capable.

>    - Create a set of pre-configured inter-connected VM images for running
> automated tests?  If so, what format, what CPU architecture, etc

It seems this, or something like it, would be necessary. I've got
some convenience scripts that I'm using to boot and test kernels, you
can find them in the debugging repository I've listed previously [1].

If you'd like, maybe we can use that repository to start putting
together some sort of testing framework.

[1]: https://github.com/larsks/ax25-debugging

> Are you patching and compiling the Raspberry Pi OS specific kernel tree or
> are you using the mainline Linux-Next or primary Linus tree?

The 6.6.30 patch is against https://github.com/raspberrypi/linux (the
rpi-6.6.y branch); I didn't think it was possible to boot the mainline
kernel on the Pi.

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

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Tue, May 21, 2024 at 02:12:06PM GMT, Lars Kellogg-Stedman wrote:
> On Tue, May 21, 2024 at 08:53:06AM GMT, David Ranch wrote:
> > I am hugely grateful to you (Lars) on being able to submit some meaningful
> > fixes and was able to get them committed to the mainline.
> 
> Just to clear, that hasn't happened yet! The patch needs to get
> approved first.

The patch has been merged into the mainline kernel:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3c34fb0bd4a4237592c5ecb5b2e2531900c55774

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

Re: Kernel 6.9.1 AX.25 Crash

[David Ranch]

Hello Lars,

Thank you and all that helped get this fix into the kernel!  Beyond the 
fix being available in 6.9.1, what can be done for the various LTS 
kernels?  I'm personally very interested in seeing this get into the 
Raspberry Pi Bookworm 6.6.x kernels.

--David
KI6ZHD


On 06/07/2024 05:01 AM, Lars Kellogg-Stedman wrote:
> On Tue, May 21, 2024 at 02:12:06PM GMT, Lars Kellogg-Stedman wrote:
>> On Tue, May 21, 2024 at 08:53:06AM GMT, David Ranch wrote:
>>> I am hugely grateful to you (Lars) on being able to submit some meaningful
>>> fixes and was able to get them committed to the mainline.
>>
>> Just to clear, that hasn't happened yet! The patch needs to get
>> approved first.
>
> The patch has been merged into the mainline kernel:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3c34fb0bd4a4237592c5ecb5b2e2531900c55774
>

Re: Kernel 6.9.1 AX.25 Crash

[Lars Kellogg-Stedman]

On Fri, Jun 07, 2024 at 06:49:57AM GMT, David Ranch wrote:
> I'm personally very interested in seeing this get into the Raspberry
> Pi Bookworm 6.6.x kernels.

I've submitted a pull request [1] against the Raspberry Pi 6.6.x kernel
tree. I don't know if that's the right way to do it or not; I guess
we'll find out.

[1]: https://github.com/raspberrypi/linux/pull/6213

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

Re: Kernel 6.9.1 AX.25 Crash

[David Ranch]

Thank you for doing that Lars!  Thinking of other packet users, do you 
know of an easy way to request this for other distros?  I know a lot of 
packet users use Mint, Ubuntu, etc.  All this kernel tree fragmentation 
across distros is a mess but it's the reality of what Linux is these days.

--David


On 06/07/2024 06:59 PM, Lars Kellogg-Stedman wrote:
> On Fri, Jun 07, 2024 at 06:49:57AM GMT, David Ranch wrote:
>> I'm personally very interested in seeing this get into the Raspberry
>> Pi Bookworm 6.6.x kernels.
>
> I've submitted a pull request [1] against the Raspberry Pi 6.6.x kernel
> tree. I don't know if that's the right way to do it or not; I guess
> we'll find out.
>
> [1]: https://github.com/raspberrypi/linux/pull/6213
>

Re: Kernel 6.9.1 AX.25 Crash

[Dan Carpenter]

I sent an alternative patch which isn't totally complete and hasn't been
reviewed.

https://lore.kernel.org/all/79dc1067-76dc-43b2-9413-7754f96fe08e@moroto.mountain/

But really, if we're going to bump the refcount on accept then we do
need to add the ax25_dev_put(ax25->ax25_dev) in ax25_cb_put().

The part of my patch which isn't complete is that we probably need a
netdev_hold/put() as well I guess.  I don't know what that does.

regards,
dan carpenter

Re: Kernel 6.9.1 AX.25 Crash

[Chris Maness]

Now running this kernel on an otherwise stock Slackware-15 system.  It
is working FB.  The stock kernel 5.15.145 works for incoming
connections, but it does not release the socket if I make an outbound
connection and crash out of the remote end leaving my outbound socket
orphaned.  With the 6.9.1 kernel (with lars inbound patch), the
orphaned socket clears after about 5minutes, and I would consider that
a normal function.

On Mon, May 20, 2024 at 7:42 AM Lars Kellogg-Stedman <lars@oddbit.com> wrote:
>
> On Sun, May 19, 2024 at 07:52:41PM GMT, Chris Maness wrote:
> > Since you guys have been working on this, I have started playing around
> > with slackware-current to check on your progress.  I setup LinFBB with one
> > pseudo tty  port generated with ax25ipd.  It does release the socket after
> > connection, and worked for a couple of hours.  However, it crashed hard
> > with no terminal dump when I was forwarding some bulls over AX.25 over UDP
> > to the ax25ipd.  This does involve the AX.25 stack as FBB needs an ax0 port.
>
> Chris,
>
> At the moment, the Linux ax.25 stack is unusable for handling incoming
> connections (outbound connections seem to be stable).
>
> I posted a patch to this list at the end of April that has completely
> resolved the ax.25 crashing problems for me. I'm running it on x86_64
> systems with kernel 6.9.0, and on Raspberry Pi systems running 6.6.30.
>
> Dan was unhappy with the patch, and Duoming has been silent on this
> topic, so I'm not sure how to proceed. I am confident that there is a
> "more correct" solution to this problem, but I am also confident that
> this patch corrects a real issue; without a more experienced developer
> either suggesting changes to this patch or submitting their own I don't
> know if this will move forward. Duoming indicated some interest  in
> the issue on 5/15 [1], but I haven't heard anything since then.
>
> I've included the current version of the patch in this email. If you
> have the chance to try it out, I'm curious to know whether or not it
> solves your problems.
>
> Cheers,
>
> -- Lars
>
> [1] https://marc.info/?l=linux-kernel&m=171576662414653&w=2
>
> >8------------------------------------------------------8<
>
> When closing a socket in ax25_release(), we call netdev_put() to decrease
> the refcount on the ax.25 device. However, the execution path for accepting
> an incoming connection never calls ax25_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 ax1 to become free. Usage count = 0
>
> The attached patch corrects these issues by ensuring that we call
> netdev_hold() and ax25_dev_hold() in ax25_accept().
>
> 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 9169efb2f43..5aa6e5c3495 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, int flags,
>         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, int flags,
>         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 net] ax25: Fix refcount leak issues of ax25_dev

[Dan Carpenter]

On Wed, May 01, 2024 at 02:02:18PM +0800, Duoming Zhou wrote:
> @@ -58,7 +59,6 @@ void ax25_dev_device_up(struct net_device *dev)
>  		return;
>  	}
>  
> -	refcount_set(&ax25_dev->refcount, 1);

Let's keep this here, and just delete the ax25_dev_hold().  It makes
the diff smaller and I like setting the refcount earlier anyway.

>  	dev->ax25_ptr     = ax25_dev;

Let's move this assignment under the spinlock where ax25_dev_hold() was.

>  	ax25_dev->dev     = dev;
>  	netdev_hold(dev, &ax25_dev->dev_tracker, GFP_KERNEL);
> @@ -88,7 +88,7 @@ void ax25_dev_device_up(struct net_device *dev)
>  	ax25_dev->next = ax25_dev_list;
>  	ax25_dev_list  = ax25_dev;
>  	spin_unlock_bh(&ax25_dev_lock);
> -	ax25_dev_hold(ax25_dev);
> +	refcount_set(&ax25_dev->refcount, 1);
>  
>  	ax25_register_dev_sysctl(ax25_dev);
>  }
> @@ -135,7 +135,6 @@ void ax25_dev_device_down(struct net_device *dev)
>  
>  unlock_put:
>  	spin_unlock_bh(&ax25_dev_lock);
> -	ax25_dev_put(ax25_dev);
>  	dev->ax25_ptr = NULL;
>  	netdev_put(dev, &ax25_dev->dev_tracker);
>  	ax25_dev_put(ax25_dev);

So far as I can see, the ax25_dev should be on the list.  Also, I think
the dev->ax25_ptr = NULL; assignment should be under the lock.  So this
code should just look like:

        list_for_each_entry(s, &ax25_dev_list, list) {
                if (s->forward == dev)
                        s->forward = NULL;
        }

        list_for_each_entry(s, &ax25_dev_list, list) {
                if (s == ax25_dev) {
                        list_del(&s->list);
                        break;
                }
        }
        dev->ax25_ptr = NULL;
        spin_unlock_bh(&ax25_dev_lock);
        netdev_put(dev, &ax25_dev->dev_tracker);
        ax25_dev_put(ax25_dev);
}

Also it should just be on the list once...  In fact, it's impossible for
one pointer to be on a list twice.  So it would be nice to add a break;
in ax25_addr_ax25dev().  It doesn't change the code, it just makes it
more obvious.

ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
{
        ax25_dev *ax25_dev, *res = NULL;

        spin_lock_bh(&ax25_dev_lock);
        list_for_each_entry(ax25_dev, &ax25_dev_list, list) {
                if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
                        res = ax25_dev;
			ax25_dev_hold(res);
                        break;
                }
        }
        spin_unlock_bh(&ax25_dev_lock);

        return res;
}

regards,
dan carpenter

Kernel 6.9.1 AX.25 Crash

[Chris Maness]

Since you guys have been working on this, I have started playing
around with slackware-current to check on your progress.  I setup
LinFBB with one pseudo tty  port generated with ax25ipd.  It does
release the socket after connection, and worked for a couple of hours.
However, it crashed hard with no terminal dump when I was forwarding
some bulls over AX.25 over UDP to the ax25ipd.  This does involve the
AX.25 stack as FBB needs an ax0 port.

If anyone is interested in reproducing, I can give you the details on
how I triggered it.

-Chris KQ6UP

-- 
Thanks,
Chris Maness