netdev - Re: [PATCH RFC 1/2] flow: virtualize get and entry deletion methods

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: <20100329090927.GA22899@gondor.apana.org.au>
Date:	Mon, 29 Mar 2010 17:09:27 +0800
From:	Herbert Xu <herbert@...dor.apana.org.au>
To:	Timo Teräs <timo.teras@....fi>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC 1/2] flow: virtualize get and entry deletion methods

On Mon, Mar 29, 2010 at 12:00:38PM +0300, Timo Teräs wrote:
>
>> Timo, what was the reason for getting rid of the synchronous
>> flush again?
>
> No, just having the flush call is not enough to guarantee
> liveliness. The flushing happens in delayed work, and the flows
> might be in use before the flush has been finished or even
> started.
>
> I was also hoping to move the "delayed" deletion part to
> flow cache core so the code would be shared with policies and
> bundles.
>
> As stated before, we don't really need lock for the 'dead' check.
> It's only written once, and actually read without lock in some
> other places too. And all the other places that do take the lock,
> release it right away making the resulting dead check result
> "old" anyway. 

No that's not the point.

The lock is not there to protect reading ->dead, which is atomic
anyway.

It's there to guarantee that you don't increment the ref count
on a dead policy.

For the flow cache we didn't need this because the policy code
would flush the cache synchronously so we can always grab a ref
count safely as long as BH is still off.

So if you leave the flow cache flushing as is, then we should
still be able to do the it without holding the lock, or checking
for deadness.

> Looks to me that the whole policy locking is not up-to-date
> anymore. Apparently the only place that actually needs it is
> the migration code (which just happens to be broke anyway since
> bundle creation does not take read lock). But it could be
> relatively easily changed to replace policy with new
> templates... and the whole policy stuff converted to RCU.

I wouldn't be surprised that the migration code is buggy.  But
that's orthogonal to your patch.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
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