netdev - Re: [PATCH] xfrm: cache bundle lookup results in flow cache

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: <4BA86DA1.7090707@iki.fi>
Date:	Tue, 23 Mar 2010 09:28:33 +0200
From:	Timo Teräs <timo.teras@....fi>
To:	Herbert Xu <herbert@...dor.apana.org.au>
CC:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] xfrm: cache bundle lookup results in flow cache

Timo Teräs wrote:
> Herbert Xu wrote:
>> On Sun, Mar 21, 2010 at 10:31:23AM +0200, Timo Teräs wrote:
>>>> Ok, we can do that to skip 2. But I think 1 would be still useful.
>>>> It'd probably be good to actually have flow_cache_ops pointer in
>>>> each entry instead of the atomic_t pointer.
>>>>
>>>> The reasoning:
>>>> - we can then have type-based checks that the reference count
>>>>  is valid (e.g. policy's refcount must not go to zero, it's bug,
>>>>  and we can call dst_release which warns if refcount goes to
>>>>  negative); imho it's hack to call atomic_dec instead of the
>>>>  real type's xxx_put
>>>> - the flow cache needs to somehow know if the entry is stale so
>>>>  it'll try to refresh it atomically; e.g. if there's no
>>>>  check for 'stale', the lookup returns stale xfrm_dst. we'd
>>>>  then need new api to update the stale entry, or flush it out
>>>>  and repeat the lookup. the virtual get could check for it being
>>>>  stale (if so release the entry) and then return null for the
>>>>  generic code to call the resolver atomically
>>>> - for paranoia we can actually check the type of the object in
>>>>  cache via the ops (if needed)
>>
>> The reason I'd prefer to keep the current scheme is to avoid
>> an additional indirect function call on each packet.
>>
>> The way it would work is (we need flow_cache_lookup to return
>> fle instead of the object):
>>
>>     fle = flow_cache_lookup
>>     xdst = fle->object
>>     if (xdst is stale) {
>>         flow_cache_mark_obsolete(fle)
>>         fle = flow_cache_lookup
>>         xdst = fle->object
>>         if (xdst is stale)
>>             return error
>>     }

I've been thinking more about doing this. And I think this
approach is fundamentally racy.

Underlying fle->object can be changed underneath as since
bh are not disabled. This means the refcounting needs to
be done on fle level, and additional measures are needed
to ensure that fle->object is as expected.

Additionally, the second flow_cache_lookup can happen
on another cpu, and could return a stale object that
indeed would need refreshing instead of generating an
error.

Options:
1. return fle and fle->object separately, refcount both
2. call flow_cache_lookup with bh disabled
3. use flow_cache_entry_ops and virtualize put/get

Since 2. was previously said to leak generic code,
it does not sound good.

On 1 vs 3, I'd still choose 3 since:
- adding one more refcount means more atomic calls
  which have same (or more) overhead as indirect call
- two refcounts sounds more complicated than one
- exposing fle and touching it without bh's disabled
  requires a whole lot of more extra care; doing
  3 is simpler

So, should I go ahead and do the virtualization of the
getters and putters?

- 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