[PATCH net v5 0/4] ax25: Fix issues of ax25_dev and net_device

[PATCH net v5 0/4] ax25: Fix issues of ax25_dev and net_device

[Duoming Zhou]

The first patch uses kernel universal linked list to implement
ax25_dev_list, which makes the operation of the list easier.
The second and third patch fix reference count leak issues of
the object "ax25_dev" and "net_device". The last patch uses
ax25_dev_put() to replace kfree() in ax25_dev_free().

You can see the former discussion in the following link:
https://lore.kernel.org/netdev/20240501060218.32898-1-duoming@zju.edu.cn/

Duoming Zhou (4):
  ax25: Use kernel universal linked list to implement ax25_dev_list
  ax25: Fix reference count leak issues of ax25_dev
  ax25: Fix reference count leak issues of net_device
  ax25: Change kfree() in ax25_dev_free() to ax25_dev_put()

 include/net/ax25.h  |  3 +--
 net/ax25/ax25_dev.c | 52 +++++++++++++++++----------------------------
 2 files changed, 20 insertions(+), 35 deletions(-)

-- 
2.17.1

[PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Duoming Zhou]

The origin ax25_dev_list implements its own single linked list,
which is complicated and error-prone. For example, when deleting
the node of ax25_dev_list in ax25_dev_device_down(), we have to
operate on the head node and other nodes separately.

This patch uses kernel universal linked list to replace original
ax25_dev_list, which make the operation of ax25_dev_list easier.
There are two points that need to notice:

[1] We should add a check to judge whether the list is empty before
INIT_LIST_HEAD in ax25_dev_device_up(), otherwise it will empty the
list for each new ax25_dev added.

[2] We should do "dev->ax25_ptr = ax25_dev;" and "dev->ax25_ptr = NULL;"
while holding the spinlock, otherwise the ax25_dev_device_up() and
ax25_dev_device_down() could race, we're not guaranteed to find a match
ax25_dev in ax25_dev_device_down().

Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 include/net/ax25.h  |  3 +--
 net/ax25/ax25_dev.c | 43 ++++++++++++++++++-------------------------
 2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/include/net/ax25.h b/include/net/ax25.h
index 0d939e5aee4..c2a85fd3f5e 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 282ec581c07..1557f879377 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,7 +35,7 @@ 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);
@@ -52,6 +53,9 @@ void ax25_dev_device_up(struct net_device *dev)
 {
 	ax25_dev *ax25_dev;
 
+	/* Initialized the list for the first entry */
+	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");
@@ -59,7 +63,6 @@ void ax25_dev_device_up(struct net_device *dev)
 	}
 
 	refcount_set(&ax25_dev->refcount, 1);
-	dev->ax25_ptr     = ax25_dev;
 	ax25_dev->dev     = dev;
 	netdev_hold(dev, &ax25_dev->dev_tracker, GFP_KERNEL);
 	ax25_dev->forward = NULL;
@@ -85,8 +88,8 @@ 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);
+	dev->ax25_ptr     = ax25_dev;
 	spin_unlock_bh(&ax25_dev_lock);
 	ax25_dev_hold(ax25_dev);
 
@@ -111,32 +114,25 @@ 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;
+	spin_unlock_bh(&ax25_dev_lock);
 	ax25_dev_put(ax25_dev);
 	return;
 
 unlock_put:
+	dev->ax25_ptr = NULL;
 	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);
 }
@@ -200,16 +196,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);
 }
-- 
2.17.1

[PATCH net v5 2/4] ax25: Fix reference count leak issues of ax25_dev

[Duoming Zhou]

The ax25_addr_ax25dev() and ax25_dev_device_down() exist a reference
count leak issue of the object "ax25_dev".

Memory leak issue in ax25_addr_ax25dev():

The reference count of the object "ax25_dev" can be increased multiple
times in ax25_addr_ax25dev(). This will cause a memory leak so far.

Memory leak issues in ax25_dev_device_down():

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

As for the issue of ax25_addr_ax25dev(), it is impossible for one pointer
to be on a list twice. So add a break in ax25_addr_ax25dev(). As for the
issue of ax25_dev_device_down(), increase the reference count of ax25_dev
once in ax25_dev_device_up() and decrease the reference count of ax25_dev
after it is removed from the ax25_dev_list.

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

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 1557f879377..66aa381af0e 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -39,6 +39,7 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr)
 		if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) {
 			res = ax25_dev;
 			ax25_dev_hold(ax25_dev);
+			break;
 		}
 	spin_unlock_bh(&ax25_dev_lock);
 
@@ -91,7 +92,6 @@ void ax25_dev_device_up(struct net_device *dev)
 	list_add(&ax25_dev->list, &ax25_dev_list);
 	dev->ax25_ptr     = ax25_dev;
 	spin_unlock_bh(&ax25_dev_lock);
-	ax25_dev_hold(ax25_dev);
 
 	ax25_register_dev_sysctl(ax25_dev);
 }
@@ -132,7 +132,6 @@ void ax25_dev_device_down(struct net_device *dev)
 unlock_put:
 	dev->ax25_ptr = NULL;
 	spin_unlock_bh(&ax25_dev_lock);
-	ax25_dev_put(ax25_dev);
 	netdev_put(dev, &ax25_dev->dev_tracker);
 	ax25_dev_put(ax25_dev);
 }
-- 
2.17.1

[PATCH net v5 3/4] ax25: Fix reference count leak issues of net_device

[Duoming Zhou]

The ax25_dev_device_down() exists reference count leak issues of
the object "net_device". When the ax25 device is shutting down.
The ax25_dev_device_down() drops the reference count of net_device
one or zero times depending on if we goto unlock_put or not, which
will cause memory leak.

In order to solve the above issue, decrease the reference count of
net_device after dev->ax25_ptr is set to null.

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

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 66aa381af0e..c6ab9b0f0be 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -121,15 +121,9 @@ void ax25_dev_device_down(struct net_device *dev)
 	list_for_each_entry(s, &ax25_dev_list, list) {
 		if (s == ax25_dev) {
 			list_del(&s->list);
-			goto unlock_put;
+			break;
 		}
 	}
-	dev->ax25_ptr = NULL;
-	spin_unlock_bh(&ax25_dev_lock);
-	ax25_dev_put(ax25_dev);
-	return;
-
-unlock_put:
 	dev->ax25_ptr = NULL;
 	spin_unlock_bh(&ax25_dev_lock);
 	netdev_put(dev, &ax25_dev->dev_tracker);
-- 
2.17.1

[PATCH net v5 4/4] ax25: Change kfree() in ax25_dev_free() to ax25_dev_put()

[Duoming Zhou]

The object "ax25_dev" is managed by reference counting. Thus it should
not be directly released by a kfree() call in ax25_dev_free(). Replace
it with a ax25_dev_put() call instead.

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

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index c6ab9b0f0be..2a40c78f6a0 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -195,7 +195,7 @@ void __exit ax25_dev_free(void)
 	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_put(s);
 	}
 	spin_unlock_bh(&ax25_dev_lock);
 }
-- 
2.17.1

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Markus Elfring]

> … that need to notice:

I suggest to improve such a wording.


> [1] We should add a check to judge whether …

Are imperative wordings more desirable for improved change descriptions?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94

Regards,
Markus

Re: [PATCH net v5 2/4] ax25: Fix reference count leak issues of ax25_dev

[Markus Elfring]

> The ax25_addr_ax25dev() and ax25_dev_device_down() exist a reference
> count leak issue of the object "ax25_dev".

Please improve this wording.

Suggestion:
   Two function implementations contained programming mistakes.
   Thus …


> Memory leak issue in ax25_addr_ax25dev():
>
> The reference count of the object "ax25_dev" can be increased multiple
> times in ax25_addr_ax25dev(). This will cause a memory leak so far.
…

* How do you think about to work with indentation in such a description
  for item enumeration?

* Would you like to add imperative wordings for improved changelogs?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94

Regards,
Markus

Re: [PATCH net v5 3/4] ax25: Fix reference count leak issues of net_device

[Markus Elfring]

> The ax25_dev_device_down() exists reference count leak issues of
> the object "net_device". When the ax25 device is shutting down.

* Please improve this wording for the final commit.

* Do you refer to a single issue here?

Regards,
Markus

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Ratheesh Kannoth]

On 2024-05-07 at 12:33:39, Duoming Zhou (duoming@zju.edu.cn) wrote:
> The origin ax25_dev_list implements its own single linked list,
> which is complicated and error-prone. For example, when deleting
> the node of ax25_dev_list in ax25_dev_device_down(), we have to
> operate on the head node and other nodes separately.
>
> This patch uses kernel universal linked list to replace original
> ax25_dev_list, which make the operation of ax25_dev_list easier.
> There are two points that need to notice:
>
> [1] We should add a check to judge whether the list is empty before
> INIT_LIST_HEAD in ax25_dev_device_up(), otherwise it will empty the
> list for each new ax25_dev added.
>
> [2] We should do "dev->ax25_ptr = ax25_dev;" and "dev->ax25_ptr = NULL;"
> while holding the spinlock, otherwise the ax25_dev_device_up() and
> ax25_dev_device_down() could race, we're not guaranteed to find a match
> ax25_dev in ax25_dev_device_down().
>
> Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> -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,7 +35,7 @@ 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);
> @@ -52,6 +53,9 @@ void ax25_dev_device_up(struct net_device *dev)
>  {
>  	ax25_dev *ax25_dev;
>
> +	/* Initialized the list for the first entry */
> +	if (!ax25_dev_list.next)
> +		INIT_LIST_HEAD(&ax25_dev_list);
if you define ax25_dev_list using 'static LIST_HEAD(ax25_dev_list)', you need this conditional check and
initialization ?

>  	ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_KERNEL);
>  	if (!ax25_dev) {
>  		printk(KERN_ERR "AX.25: ax25_dev_device_up - out of memory\n");
> @@ -59,7 +63,6 @@ void ax25_dev_device_up(struct net_device *dev)
>  	}
>
>  	refcount_set(&ax25_dev->refcount, 1);
>

Re: [PATCH net v5 4/4] ax25: Change kfree() in ax25_dev_free() to ax25_dev_put()

[Markus Elfring]

>                                          … in ax25_dev_free(). Replace
…

Can word wrapping look a bit nicer if a single word at the end will be moved
into the subsequent text line?

Regards,
Markus

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Dan Carpenter]

On Tue, May 07, 2024 at 02:59:17PM +0530, Ratheesh Kannoth wrote:
> On 2024-05-07 at 12:33:39, Duoming Zhou (duoming@zju.edu.cn) wrote:
> > The origin ax25_dev_list implements its own single linked list,
> > which is complicated and error-prone. For example, when deleting
> > the node of ax25_dev_list in ax25_dev_device_down(), we have to
> > operate on the head node and other nodes separately.
> >
> > This patch uses kernel universal linked list to replace original
> > ax25_dev_list, which make the operation of ax25_dev_list easier.
> > There are two points that need to notice:
> >
> > [1] We should add a check to judge whether the list is empty before
> > INIT_LIST_HEAD in ax25_dev_device_up(), otherwise it will empty the
> > list for each new ax25_dev added.
> >
> > [2] We should do "dev->ax25_ptr = ax25_dev;" and "dev->ax25_ptr = NULL;"
> > while holding the spinlock, otherwise the ax25_dev_device_up() and
> > ax25_dev_device_down() could race, we're not guaranteed to find a match
> > ax25_dev in ax25_dev_device_down().
> >
> > Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > -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,7 +35,7 @@ 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);
> > @@ -52,6 +53,9 @@ void ax25_dev_device_up(struct net_device *dev)
> >  {
> >  	ax25_dev *ax25_dev;
> >
> > +	/* Initialized the list for the first entry */
> > +	if (!ax25_dev_list.next)
> > +		INIT_LIST_HEAD(&ax25_dev_list);
> if you define ax25_dev_list using 'static LIST_HEAD(ax25_dev_list)', you need this conditional check and
> initialization ?
> 

Ah, yes.  That's the proper way to do it.

regards,
dan carpenter

Re: [PATCH net v5 0/4] ax25: Fix issues of ax25_dev and net_device

[Markus Elfring]

…
>  include/net/ax25.h  |  3 +--
>  net/ax25/ax25_dev.c | 52 +++++++++++++++++----------------------------
>  2 files changed, 20 insertions(+), 35 deletions(-)

Did you accidentally overlook to provide corresponding patch version descriptions
(or changelogs) for this change iteration?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n725

Regards,
Markus

Re: [PATCH net v5 4/4] ax25: Change kfree() in ax25_dev_free() to ax25_dev_put()

[Ratheesh Kannoth]

On 2024-05-07 at 12:33:42, Duoming Zhou (duoming@zju.edu.cn) wrote:
> The object "ax25_dev" is managed by reference counting. Thus it should
> not be directly released by a kfree() call in ax25_dev_free(). Replace
> it with a ax25_dev_put() call instead.
>
> Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
> Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
>  net/ax25/ax25_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index c6ab9b0f0be..2a40c78f6a0 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -195,7 +195,7 @@ void __exit ax25_dev_free(void)
>  	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_put(s);
The commit message "The object "ax25_dev" is managed by reference counting"
seems be not making sense here.  in case ref > 0 after the ax25_dev_put().
ax25_dev_put(s) is not initiating any mechanism to come back and recheck.

>  	}
>  	spin_unlock_bh(&ax25_dev_lock);
>  }
> --
> 2.17.1
>

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Lars Kellogg-Stedman]

On Tue, May 07, 2024 at 03:03:39PM GMT, Duoming Zhou wrote:
>  typedef struct ax25_dev {
> -	struct ax25_dev		*next;
> +	struct list_head	list;

Would it make sense to replace this with:

LIST_HEAD(ax25_dev_list);

And then get rid of:

> +	/* Initialized the list for the first entry */
> +	if (!ax25_dev_list.next)
> +		INIT_LIST_HEAD(&ax25_dev_list);

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

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Lars Kellogg-Stedman]

On Tue, May 07, 2024 at 03:43:11PM GMT, Lars Kellogg-Stedman wrote:
> On Tue, May 07, 2024 at 03:03:39PM GMT, Duoming Zhou wrote:
> >  typedef struct ax25_dev {
> > -	struct ax25_dev		*next;
> > +	struct list_head	list;
> 
> Would it make sense to replace this with:
>
> LIST_HEAD(ax25_dev_list);

Sorry, *this*:

> +static struct list_head ax25_dev_list;


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

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Dan Carpenter]

Let's drop all the other netdev people from the CC list.

On Tue, May 07, 2024 at 07:46:51PM -0400, Lars Kellogg-Stedman wrote:
> On Tue, May 07, 2024 at 03:43:11PM GMT, Lars Kellogg-Stedman wrote:
> > On Tue, May 07, 2024 at 03:03:39PM GMT, Duoming Zhou wrote:
> > >  typedef struct ax25_dev {
> > > -	struct ax25_dev		*next;
> > > +	struct list_head	list;
> > 
> > Would it make sense to replace this with:
> >
> > LIST_HEAD(ax25_dev_list);
> 
> Sorry, *this*:
> 
> > +static struct list_head ax25_dev_list;

I'm not sure I understand.  The code is correct though...

regards,
dan carpenter

Re: [PATCH net v5 4/4] ax25: Change kfree() in ax25_dev_free() to ax25_dev_put()

[Dan Carpenter]

On Tue, May 07, 2024 at 07:43:26PM +0530, Ratheesh Kannoth wrote:
> On 2024-05-07 at 12:33:42, Duoming Zhou (duoming@zju.edu.cn) wrote:
> > The object "ax25_dev" is managed by reference counting. Thus it should
> > not be directly released by a kfree() call in ax25_dev_free(). Replace
> > it with a ax25_dev_put() call instead.
> >
> > Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
> > Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> >  net/ax25/ax25_dev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> > index c6ab9b0f0be..2a40c78f6a0 100644
> > --- a/net/ax25/ax25_dev.c
> > +++ b/net/ax25/ax25_dev.c
> > @@ -195,7 +195,7 @@ void __exit ax25_dev_free(void)
> >  	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_put(s);
> The commit message "The object "ax25_dev" is managed by reference counting"
> seems be not making sense here.  in case ref > 0 after the ax25_dev_put().
> ax25_dev_put(s) is not initiating any mechanism to come back and recheck.

The other place where we have a reference is when it's saved as
ax25->ax25_dev.

regards,
dan carpenter

Re: [PATCH net v5 1/4] ax25: Use kernel universal linked list to implement ax25_dev_list

[Lars Kellogg-Stedman]

On Wed, May 08, 2024 at 12:26:59PM GMT, Dan Carpenter wrote:
> Let's drop all the other netdev people from the CC list.
> 
> On Tue, May 07, 2024 at 07:46:51PM -0400, Lars Kellogg-Stedman wrote:
> > On Tue, May 07, 2024 at 03:43:11PM GMT, Lars Kellogg-Stedman wrote:
> > > On Tue, May 07, 2024 at 03:03:39PM GMT, Duoming Zhou wrote:
> > > >  typedef struct ax25_dev {
> > > > -	struct ax25_dev		*next;
> > > > +	struct list_head	list;
> > > 
> > > Would it make sense to replace this with:
> > >
> > > LIST_HEAD(ax25_dev_list);
> > 
> > Sorry, *this*:
> > 
> > > +static struct list_head ax25_dev_list;
> 
> I'm not sure I understand.  The code is correct though...

I was suggested using:

    LIST_HEAD(list_name);

Rather than:

    static struct list_head list_name;

...and then later on initializing the list using
INIT_LIST_HEAD(list_name).

There's not really a functional difference, but using LIST_HEAD saves us
a couple of extra lines of initialization code.

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