netdev - Re: xfrm_state locking regression...

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: <20080902.233538.200370430.davem@davemloft.net>
Date:	Tue, 02 Sep 2008 23:35:38 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	timo.teras@....fi
Cc:	herbert@...dor.apana.org.au, netdev@...r.kernel.org
Subject: Re: xfrm_state locking regression...

From: Timo Teräs <timo.teras@....fi>
Date: Wed, 03 Sep 2008 09:27:47 +0300

> Well, it's just another list keeping a reference like ->bydst,
> ->bysrc and ->byspi. The actual amount of external references is
> still correct (the walking code calls _hold() when it returns while
> keeping an external pointer).

->bydst, ->bysrc, and ->byspi are counted as a single external
reference because:

1) They are controlled as a group

2) Doing 3 atomic operations is more expensive than one

I know because I did that conversion from 3 refcount operations down
to 1 and I timed it with stress tests, which showed that it made a
huge performance difference for the control path of our IPSEC stack.

> The difference is that node should not be unlinked from ->all until
> all other references are gone. For other lists the unlinking can be
> done earlier since they are used only for lookups.

Once there are no list references, there cannot be any other references.
So in fact it seems to me that unlinking when the xfrm_state is removed
from those other lists makes perfect sense.

If __xfrm_state_delete sets the state to DEAD, and you skip xfrm_state
objects marked DEAD, why does the ->all list reference have to survive
past __xfrm_state_delete()?

It seems the perfect place to do the ->all removal.

> Any good other ways to enumerate to list entries while allowing
> to keep a temporary "iterator"? The previous method was crap too.

At least the old stuff was self-consistent and only needed one central
lock grab to destoy an object.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html