netdev - Re: [PATCH net-next 06/15] net: ax25: remove route refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220127091858.GF18529@x-berg.in-berlin.de>
Date:   Thu, 27 Jan 2022 10:18:58 +0100
From:   Thomas Osterried <thomas@...erried.de>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, ralf@...ux-mips.org,
        linux-hams@...r.kernel.org
Subject: Re: [PATCH net-next 06/15] net: ax25: remove route refcount

Hello,

 think it's absolutely correct to state
  "Nothing takes the refcount since v4.9."
because in ax25_rt_add(),
  refcount_set(&ax25_rt->refcount, 1);
is used (for every new ax25_rt entry).

But nothing does an increment.
There's one function in include/net/ax25.h ,
  ax25_hold_route() which would refcount_inc(&ax25_rt->refcount)
but it's not called from anywhere.

=> It's value is always 1, and the deleting function ax25_put_route() decrements it again before freeing.
   I also see no sense in this (anymore).

Every struct ax25_route_list operation is assured with either
  write_lock_bh(&ax25_route_lock);
  write_unlock_bh(&ax25_route_lock);
or
  the struct ax25_route returned from functions is assured by calling read_lock(&ax25_route_lock).

-> No refcount is needed.

=> It's good to tidy up stuff that's needed anymore.

But keep in mind:
The code has strong similarities with include/net/x25.h and x25/x25_route.c ,
especially in the parts of ax25_hold_route() and ax25_rt_add().
This will get lost.

But there a things a bot does not know: human readable senteces.
ax25_get_route() is introduced with:

  /*
   *      Find AX.25 route
   *
   *      Only routes with a reference count of zero can be destroyed.
   *      Must be called with ax25_route_lock read locked.
   */

The first sentence informs: ax25_rt entries may be freed during the ax25_route_list operation.
It mentiones reference count (which will exist anymore).
The conclusion of the first sentence is "Must be called with ax25_route_lock read locked.". This is still true and assured.
I don't think it has to explain why the read lock is necessary (it's obvious, that routes could be deleted or added to the list). ->

  /*
   *      Find AX.25 route
   *
   *      Must be called with ax25_route_lock read locked.
   */

should be enough.

ff-topic:
=========

About read_lock)(): Inconsistent use.
It's
  directly called,
and by
  ax25_route_lock_use(), which calls read_lock():
  {
          read_lock(&ax25_route_lock);
  }

This makes the code harder to read.
There's also a function ax25_rt_seq_stop() that calls read_unlock() instead of calling ax25_route_lock_unuse(), which does the same.

vy 73,
	- Thomas  dl9sau

On Wed, Jan 26, 2022 at 11:11:00AM -0800, Jakub Kicinski wrote:
> Nothing takes the refcount since v4.9.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: ralf@...ux-mips.org
> CC: linux-hams@...r.kernel.org
> ---
>  include/net/ax25.h    | 12 ------------
>  net/ax25/ax25_route.c |  5 ++---
>  2 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/ax25.h b/include/net/ax25.h
> index 526e49589197..cb628c5d7c5b 100644
> --- a/include/net/ax25.h
> +++ b/include/net/ax25.h
> @@ -187,18 +187,12 @@ typedef struct {
>  
>  typedef struct ax25_route {
>  	struct ax25_route	*next;
> -	refcount_t		refcount;
>  	ax25_address		callsign;
>  	struct net_device	*dev;
>  	ax25_digi		*digipeat;
>  	char			ip_mode;
>  } ax25_route;
>  
> -static inline void ax25_hold_route(ax25_route *ax25_rt)
> -{
> -	refcount_inc(&ax25_rt->refcount);
> -}
> -
>  void __ax25_put_route(ax25_route *ax25_rt);
>  
>  extern rwlock_t ax25_route_lock;
> @@ -213,12 +207,6 @@ static inline void ax25_route_lock_unuse(void)
>  	read_unlock(&ax25_route_lock);
>  }
>  
> -static inline void ax25_put_route(ax25_route *ax25_rt)
> -{
> -	if (refcount_dec_and_test(&ax25_rt->refcount))
> -		__ax25_put_route(ax25_rt);
> -}
> -
>  typedef struct {
>  	char			slave;			/* slave_mode?   */
>  	struct timer_list	slave_timer;		/* timeout timer */
> diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c
> index d0b2e094bd55..be97dc6a53cb 100644
> --- a/net/ax25/ax25_route.c
> +++ b/net/ax25/ax25_route.c
> @@ -111,7 +111,6 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
>  		return -ENOMEM;
>  	}
>  
> -	refcount_set(&ax25_rt->refcount, 1);
>  	ax25_rt->callsign     = route->dest_addr;
>  	ax25_rt->dev          = ax25_dev->dev;
>  	ax25_rt->digipeat     = NULL;
> @@ -160,12 +159,12 @@ static int ax25_rt_del(struct ax25_routes_struct *route)
>  		    ax25cmp(&route->dest_addr, &s->callsign) == 0) {
>  			if (ax25_route_list == s) {
>  				ax25_route_list = s->next;
> -				ax25_put_route(s);
> +				__ax25_put_route(s);
>  			} else {
>  				for (t = ax25_route_list; t != NULL; t = t->next) {
>  					if (t->next == s) {
>  						t->next = s->next;
> -						ax25_put_route(s);
> +						__ax25_put_route(s);
>  						break;
>  					}
>  				}
> -- 
> 2.34.1
>