On Mon, Oct 29, 2018 at 05:54:25PM +0100, Phil Sutter wrote:
> Hi,
>
> On Mon, Oct 29, 2018 at 04:19:03PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Oct 26, 2018 at 03:01:38PM +0200, Phil Sutter wrote:
> > > The basic principle is to not return a JSON object freshly created from
> > > netlink responses, but just update the existing user-provided one to
> > > make sure callers get back exactly what they expect.
> >
> > Applied, thanks Phil.
> >
> > > To achieve that, keep the parsed JSON object around in a global variable
> > > ('cur_root') and provide a custom callback to insert handles into it
> > > from received netlink messages. The tricky bit here is updating rules
> > > since unique identification is problematic. Therefore drop possibly
> > > present handles from input and later assume updates are received in
> > > order so the first rule not having a handle set is the right one.
> >
> > You can use the netlink sequence number to correlate the rule that you
> > are adding with the object that the kernel gives us. The kernel will
> > reply including the sequence number of the original request in the
> > reply you get. Like this, you will not need obj_info_matches() and
> > such I think. In case this helps you simplify this code.
>
> That's an interesting idea, but I don't think it would fly: netlink
> seqnum is per batch and potentially there are multiple add commands
> contained in it.
netlink seqnum allow us to know what command in a batch is causing
problems, so there should be one unique sequence number per command.
Assuming a batch with two commands, each command will get a different
sequence number. This is how we can correlate what line in a file is
causing what problem when we load a batch via nft -f. The kernel
doesn't stop on the first error, so it keeps trying so users get
a listing of commands causing errors in one shot [1].
> So although I could make sure the kernel's reply matches the current
> request, I still have to figure out which JSON add command to
> update. Or am I missing something?
You need to keep a list of commands, that store json object that you
send to the kernel and its netlink sequence number, so you can use to
find a matching coming as reply. This should be similar to
nft_netlink(), we could event generalize it so it can be re-used from
the json codebase too.
Does this clarify?
[1] Note that we still have problems with runtime lookups that results
in table/chain/etc does not exist, ie. table_lookup() calls from
userspace using the cache, we could create error table/chain/set
objects so we can keep processing the batch on unexisting
tables/chains/sets and send everything to the kernel, then get all
errors in one go. This is convenient so users developing a firewall
policy don't have to hit one error at a time, basically we will get
this behaving like a real compiler which spews all errors in the code
in one go. But that is a different problem that I would like to sort
out at some point. Anyway, the kernel is not the limiting side here,
we only need to extend userspace to remove the limitation I'm
describing in this side note.
