netdev - Re: [PATCH net-next 00/21] remove dst garbage collector logic

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: <20170616.150711.596195466309167124.davem@davemloft.net>
Date:   Fri, 16 Jun 2017 15:07:11 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     tracywwnj@...il.com
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, kafai@...com,
        weiwan@...gle.com
Subject: Re: [PATCH net-next 00/21] remove dst garbage collector logic

From: Wei Wang <tracywwnj@...il.com>
Date: Fri, 16 Jun 2017 10:47:23 -0700

> From: Wei Wang <weiwan@...gle.com>
> 
> The current mechanism of dst release is a bit complicated. It is because
> the users of dst get divided into 2 situations:
>   1. Most users take the reference count when using a dst and release the
>      reference count when done.
>   2. Exceptional users like IPv4/IPv6/decnet/xfrm routing code do not take
>      reference count when referencing to a dst due to some histotic reasons.
> 
> Due to those exceptional use cases in 2, reference count being 0 is not an
> adequate evidence to indicate that no user is using this dst. So users in 1
> can't free the dst simply based on reference count being 0 because users in
> 2 might still hold reference to it.
> Instead, a dst garbage list is needed to hold the dst entries that already
> get removed by the users in 2 but are still held by users in 1. And a periodic
> garbage collector task is run to check all the dst entries in the list to see
> if the users in 1 have released the reference to those dst entries.
> If so, the dst is now ready to be freed.
> 
> This logic introduces unnecessary complications in the dst code which makes it
> hard to understand and to debug.
> 
> In order to get rid of the whole dst garbage collector (gc) and make the dst
> code more unified and simplified, we can make the users in 2 also take reference
> count on the dst and release it properly when done.
> This way, dst can be safely freed once the refcount drops to 0 and no gc
> thread is needed anymore.
> 
> This patch series' target is to completely get rid of dst gc logic and free
> dst based on reference count only.
> Patch 1-3 are preparation patches to do some cleanup/improvement on the existing
> code to make later work easier.
> Patch 4-21 are real implementations.
> In these patches, a temporary flag DST_NOGC is used to help transition
> those exceptional users one by one. Once every component is transitioned,
> this temporary flag is removed.
> By the end of this patch series, all dst are refcounted when being used
> and released when done. And dst will be freed when its refcount drops to 0.
> No dst gc task is running anymore.
> 
> Note: This patch series depends on the decnet fix that was sent right before:
>       "decnet: always not take dst->__refcnt when inserting dst into hash table"

Other than the minor feedback I gave, this series looks great!

Indeed the code is a lot simpler afterwards and much easier to audit and
understand.

I'll wait a bit for some others to give feedback as well.