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: <4BA33FF5.8010104@iki.fi> Date: Fri, 19 Mar 2010 11:12:21 +0200 From: Timo Teräs <timo.teras@....fi> To: Herbert Xu <herbert@...dor.apana.org.au> CC: netdev@...r.kernel.org Subject: Re: [PATCH] xfrm: cache bundle lookup results in flow cache Herbert Xu wrote: > On Fri, Mar 19, 2010 at 10:37:58AM +0200, Timo Teräs wrote: >> But I changed that. the flow cache now does *not* call local_bh_enable >> if it returns something. This is deferred until corresponding _put >> call. So bh's are disable while we are touching the lookup results. > > I'm sorry but making a function like flow_cache_lookup return with > BH disabled is just wrong! > >> It'd probably make sense to remove that. And require _lookup to >> be called with bh disabled so it's more obvious that bh's are >> disabled when touching the cache entry. > > That would be better but it's still hacky. Proper reference > counting like we had before would be my preference. Well, the cache entry is still referenced only very shortly, I don't see why keeping bh disabled why doing it is considered a hack. Refcounting the cache entries is trickier. Though, it could be used to optimize the update process: we could safely update it instead of doing now lookup later. >> Not a race. We need to keep bh's disabled while touching fce >> for various reasons. > > What are those reasons (apart from this race)? This. And that the cache is synchronized by flow_cache_flush executing stuff on other cpu's, ensuring that it's not running any protected cache accessing code. See below. > >> Noone. When policy and dst is on cache there's no reference. >> The cache generation id's ensure that the object exists when >> they are in cache. It might make sense to add references to >> both objects and do a BUG_ON if the flow cache flusher would >> need to delete an object. I guess this would be the proper >> way, since that's how the dst stuff works too. > > The cache genid is not enough: > > CPU1 CPU2 > check genid == OK > update genid > kill policy > kfree on policy > use policy == BOOM The sequence goes currently. CPU1 CPU2 disable_bh check genid == OK update genid call cache_flush blocks use policy == OK and take refcount enable_bh cache_flush smpcall executes and ublocks cpu2 returns from cache_flush kill policy kfree on policy - Timo -- 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
Powered by blists - more mailing lists