nft cache updates

nft cache updates

[Patrick McHardy]

Hi Pablo,

I'm wondering what the rational for the current cache update behaviour is.
The changelog states it is somehow related to the requested command, but
that doesn't seem to be true.

Even "nft describe" fails with EPERM as user since the cache appears to be
initialized unconditionally, which is a bit unfortunate. Also I used to
test things parsing, evaluation and even netlink generation without actually
adding those rules as user, which does not work anymore. This might be harder
to get working again, but I'm not sure why we do a full initialization anyways.
The only thing that appears to be needed are sets, and those only in some
specific circumstances.

Cheers,
Patrick

Re: nft cache updates

[Pablo Neira Ayuso]

On Mon, Nov 09, 2015 at 03:30:56PM +0000, Patrick McHardy wrote:
> Hi Pablo,
> 
> I'm wondering what the rational for the current cache update behaviour is.
> The changelog states it is somehow related to the requested command, but
> that doesn't seem to be true.
> 
> Even "nft describe" fails with EPERM as user since the cache appears to be
> initialized unconditionally, which is a bit unfortunate. Also I used to
> test things parsing, evaluation and even netlink generation without actually
> adding those rules as user, which does not work anymore.  This might
> be harder to get working again, but I'm not sure why we do a full
> initialization anyways.  The only thing that appears to be needed
> are sets, and those only in some specific circumstances.

To look up for the existing sets we need the existing tables and
chains, they are essential part of the object hierarchy. So this is
what we're currently dumping.

In general, we need this for incremental updates, in scenarios where
we have objects that are defined in kernelspace but userspace refers
to them.

As you said we can disable the cache in many cases, depending on the
command or if the ruleset file starts by:

flush ruleset

but I have left this out as follow up work, I just wanted to make sure
incremental updates where working, as well as the existing changes.

nft describe should be easy to restore.

Regarding inconditional check for table and chain, we have to make it
from the evaluation step in sets, so leaving other objects without
checking this seems inconsistent to me.

Another side effect of this is better error reporting to the user.

Re: nft cache updates

[Patrick McHardy]

On 09.11, Pablo Neira Ayuso wrote:
> On Mon, Nov 09, 2015 at 03:30:56PM +0000, Patrick McHardy wrote:
> > Hi Pablo,
> > 
> > I'm wondering what the rational for the current cache update behaviour is.
> > The changelog states it is somehow related to the requested command, but
> > that doesn't seem to be true.
> > 
> > Even "nft describe" fails with EPERM as user since the cache appears to be
> > initialized unconditionally, which is a bit unfortunate. Also I used to
> > test things parsing, evaluation and even netlink generation without actually
> > adding those rules as user, which does not work anymore.  This might
> > be harder to get working again, but I'm not sure why we do a full
> > initialization anyways.  The only thing that appears to be needed
> > are sets, and those only in some specific circumstances.
> 
> To look up for the existing sets we need the existing tables and
> chains, they are essential part of the object hierarchy. So this is
> what we're currently dumping.

Well, its not really necessary to have all elements present, the set handle
is enough to recreate the hierarchy. Unless we need properties of the table
or chains themselves its quite useless.

> In general, we need this for incremental updates, in scenarios where
> we have objects that are defined in kernelspace but userspace refers
> to them.
> 
> As you said we can disable the cache in many cases, depending on the
> command or if the ruleset file starts by:
> 
> flush ruleset
> 
> but I have left this out as follow up work, I just wanted to make sure
> incremental updates where working, as well as the existing changes.

I understand. But with dumping the full hierarchy it seems reasonable to
do the update in the place where we are doing it right now. But we actually
only need a very small subset of that, and if we wouldn't dump all of that
information it would actually make sense to move it to the spots that refer
to (existing) sets only.

> nft describe should be easy to restore.
> 
> Regarding inconditional check for table and chain, we have to make it
> from the evaluation step in sets, so leaving other objects without
> checking this seems inconsistent to me.

But we don't actually need any of that information, do we?

> Another side effect of this is better error reporting to the user.

I can see that it is useful, but causing an error for reporting an error
doesn't seem like the correct way to do it. I'm also generally uncertain
about checking this *beforehand*, our usual approach is that solely the
kernel is responsible for determining object existance etc. We could
also interpret context to get the same error reporting.

Last point would be overhead of dumping all that information we don't
*really* need.

Even if we say this is acceptable (I think correctly putting netlink
errors in context is better since it allows to properly report many
other errors as well), I'd prefer if it wouldn't cause a hard failure
such as EPERM.

Re: nft cache updates

[Pablo Neira Ayuso]

On Mon, Nov 09, 2015 at 05:05:11PM +0000, Patrick McHardy wrote:
> On 09.11, Pablo Neira Ayuso wrote:
> > On Mon, Nov 09, 2015 at 03:30:56PM +0000, Patrick McHardy wrote:
> > > Hi Pablo,
> > > 
> > > I'm wondering what the rational for the current cache update behaviour is.
> > > The changelog states it is somehow related to the requested command, but
> > > that doesn't seem to be true.
> > > 
> > > Even "nft describe" fails with EPERM as user since the cache appears to be
> > > initialized unconditionally, which is a bit unfortunate. Also I used to
> > > test things parsing, evaluation and even netlink generation without actually
> > > adding those rules as user, which does not work anymore.  This might
> > > be harder to get working again, but I'm not sure why we do a full
> > > initialization anyways.  The only thing that appears to be needed
> > > are sets, and those only in some specific circumstances.
> > 
> > To look up for the existing sets we need the existing tables and
> > chains, they are essential part of the object hierarchy. So this is
> > what we're currently dumping.
> 
> Well, its not really necessary to have all elements present, the set handle
> is enough to recreate the hierarchy. Unless we need properties of the table
> or chains themselves its quite useless.

Currently, we may have to bail out in the middle of an incremental
update if the set doesn't exist.  So for sets, we would have to say
sort of "This set 'x' doesn't exist, we don't have it in the cache",
while for other objects the kernel will report the error. Doesn't this
look a bit inconsistent?

> > In general, we need this for incremental updates, in scenarios where
> > we have objects that are defined in kernelspace but userspace refers
> > to them.
> > 
> > As you said we can disable the cache in many cases, depending on the
> > command or if the ruleset file starts by:
> > 
> > flush ruleset
> > 
> > but I have left this out as follow up work, I just wanted to make sure
> > incremental updates where working, as well as the existing changes.
> 
> I understand. But with dumping the full hierarchy it seems reasonable to
> do the update in the place where we are doing it right now. But we actually
> only need a very small subset of that, and if we wouldn't dump all of that
> information it would actually make sense to move it to the spots that refer
> to (existing) sets only.

Got it, we can defer the set cache population by when we refer to a
set that doesn't exist. And we infer the existing table and chains
from the set handle. Probably there is a way to avoid this.

What do we do with nft -i? We just retrieve the cache if we need it
too?

Do you think this will cover all kind of command combinations?

add rule ...
add rule ...
list ruleset

This above is not working now, but what should we expect from listing
after adding?

> > nft describe should be easy to restore.
> > 
> > Regarding inconditional check for table and chain, we have to make it
> > from the evaluation step in sets, so leaving other objects without
> > checking this seems inconsistent to me.
> 
> But we don't actually need any of that information, do we?
>
> > Another side effect of this is better error reporting to the user.
> 
> I can see that it is useful, but causing an error for reporting an error
> doesn't seem like the correct way to do it. I'm also generally uncertain
> about checking this *beforehand*, our usual approach is that solely the
> kernel is responsible for determining object existance etc. We could
> also interpret context to get the same error reporting.
>
> Last point would be overhead of dumping all that information we don't
> *really* need.

That is something that we should avoid.

> Even if we say this is acceptable (I think correctly putting netlink
> errors in context is better since it allows to properly report many
> other errors as well), I'd prefer if it wouldn't cause a hard failure
> such as EPERM.

The EPERM problem only happens when runs as user, apart from testing,
how useful can this be? This needs net admin capabilities anyway,
we'll hit this anyway if you try a command as user that refers to a
set that doesn't exists in our cache.

Let me have a look, it shouldn't be that large patch to add on-demand
set cache population (and infer table and chain cache from the set
handle).

BTW, apart from the obvious listing case, I think we'll need this
infrastructure to work with a full consistent cache when performing
ruleset transformations.