On Wed, Oct 09, 2019 at 11:37:23AM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
>
> On Tue, Oct 08, 2019 at 06:14:40PM +0200, Phil Sutter wrote:
> > Replace the simple have_cache boolean by a cache level indicator
> > defining how complete the cache is. Since have_cache indicated full
> > cache (including rules), make code depending on it check for cache level
> > NFT_CL_RULES.
> >
> > Core cache fetching routine __nft_build_cache() accepts a new level via
> > parameter and raises cache completeness to that level.
> >
> > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > ---
> > iptables/nft-cache.c | 51 +++++++++++++++++++++++++++++++-------------
> > iptables/nft.h | 9 +++++++-
> > 2 files changed, 44 insertions(+), 16 deletions(-)
> >
> > diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
> > index 5444419a5cc3b..22a87e94efd76 100644
> > --- a/iptables/nft-cache.c
> > +++ b/iptables/nft-cache.c
> > @@ -224,30 +224,49 @@ static int fetch_rule_cache(struct nft_handle *h)
> > return 0;
> > }
> >
> > -static void __nft_build_cache(struct nft_handle *h)
> > +static void __nft_build_cache(struct nft_handle *h, enum nft_cache_level level)
> > {
> > uint32_t genid_start, genid_stop;
> >
> > + if (level <= h->cache_level)
> > + return;
> > retry:
> > mnl_genid_get(h, &genid_start);
> > - fetch_table_cache(h);
> > - fetch_chain_cache(h);
> > - fetch_rule_cache(h);
> > - h->have_cache = true;
> > - mnl_genid_get(h, &genid_stop);
> >
> > + switch (h->cache_level) {
> > + case NFT_CL_NONE:
> > + fetch_table_cache(h);
> > + if (level == NFT_CL_TABLES)
> > + break;
> > + /* fall through */
> > + case NFT_CL_TABLES:
>
> If the existing level is TABLES and use wants chains, then you have to
> invalidate the existing table cache, then fetch the tables and chains
> to make sure cache is consistent. I mean, extending an existing cache
> might lead to inconsistencies.
>
> Am I missing anything?
If I'm correct, I wonder if we should go for splitting the parsing
from the evaluation phase here. Probably generate the rule by pointing
to the table and chain as string, then evaluate the ruleset update
batch to obtain the cache level in one go. This is the approach that
I had in mind with nftables, so you might avoid dumping the ruleset
over and over in an environment where dynamic updates are frequent.
The idea is to avoid fetching a cache, then canceling it by the rule
coming afterwards just because the cache is incomplete. So the cache
that is required is calculated once, then you go to the kernel and
fetch it (making sure generation number tells you that your cache is
consistent).
Makes sense to you?
