It was not clean for me because we has no extra reference for
parallel tdb_delete(). Now I understand how it should work and why
‘TDBF_DELETED’ fixed this.
Thanks for explanation.
> On 24 Nov 2021, at 17:52, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
>
> On Wed, Nov 24, 2021 at 05:12:36PM +0300, Vitaliy Makkoveev wrote:
>> Understood. But his means we encoded double unref when we calling
>> tdb_unref() just after tdb_delete(tdb). To me it looks better to avoid
>> this and rework handlers like below:
>
> I have tried this before. It creates a tdb leak. The double unref
> is correct. tdb_delete() must only be called once.
>
>> tdb_timeout(void *v)
>> {
>> struct tdb *tdb = v;
>>
>> NET_LOCK();
>> if (tdb->tdb_flags & TDBF_TIMER) {
>> /* If it's an "invalid" TDB do a silent expiration. */
>> if (!(tdb->tdb_flags & TDBF_INVALID)) {
>> ipsecstat_inc(ipsec_exctdb);
>> pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_HARD);
>> }
>> /* tdb_delete() calls tdb_unref() */
>> tdb_delete(tdb);
>> } else {
>> /* decrement refcount of the timeout argument */
>> tdb_unref(tdb);
>> }
>> NET_UNLOCK();
>> }
>>
>> Also we could rework tdb_delete() to set the 'TDBF_DELETED' flag and do
>> not tdb_unref() passed `tdb'. It's assumed that caller should unref this
>> `tdb'. I like this because we will not kill `tdb' passed to (*xf_input)()
>> and (*xf_output)().
>
> If the caller needs a tdb ref, he has to refcount it. The tdb_delete()
> decrements the refcount that was added during allocation.
>
> My next diff will add a refcount in ip_output_ipsec_lookup() to
> address this. The current diff is complicated enough. I want to
> commit something that does neither leak nor crash. After that I
> can add more refcounts. Finally I will run in parallel.
>
> I cannot put everything in one gigantic diff.
>
> bluhm
>
