| 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
| ||
|
Message-ID: <f6749b58-ab8a-094c-5e07-c5a7ca5200c9@trinnet.net>
Date: Thu, 27 Jan 2022 07:58:40 -0800
From: David Ranch <linux-hams@...nnet.net>
To: Thomas Osterried <thomas@...erried.de>,
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
Curious, has this change been tested with an actual testbed to confirm
it doesn't break anything? We *have* to stop allowing good intentioned
but naive developers from submitting patches when they've never tested
the resulting change.
--David
KI6ZHD
On 01/27/2022 01:18 AM, Thomas Osterried wrote:
> 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
>>
Powered by blists - more mailing lists