Re: [nft PATCH] libnftables: Fix for multiple context instances

[Phil Sutter <phil@nwl.cc>]

On Mon, Nov 20, 2017 at 05:53:13PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Nov 20, 2017 at 04:58:22PM +0100, Phil Sutter wrote:
> > On Mon, Nov 20, 2017 at 02:07:32PM +0100, Pablo Neira Ayuso wrote:
> > > On Mon, Nov 20, 2017 at 01:54:18PM +0100, Phil Sutter wrote:
> > > > On Mon, Nov 20, 2017 at 01:37:26PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Thu, Nov 16, 2017 at 08:10:24PM +0100, Phil Sutter wrote:
> > > > > > If a second context is created, the second call to nft_ctx_free() leads
> > > > > > to freeing invalid pointers in nft_exit(). Fix this by introducing a
> > > > > > reference counter so that neither nft_init() nor nft_exit() run more
> > > > > > than once in a row.
> > > > > > 
> > > > > > This reference counter has to be protected from parallel access. Do this
> > > > > > using a mutex in a way that once nft_init() returns, the first call to
> > > > > > that function running in parallel is guaranteed to be finished -
> > > > > > otherwise it could happen that things being initialized in one thread
> > > > > > are already accessed in another one.
> > > > > 
> > > > > I would prefer this table is placed into the context object, so they
> > > > > are not global anymore. So I would prefer we fix this the right way(tm).
> > > > > 
> > > > > Let me know your thoughts, thanks!
> > > > 
> > > > I don't think that's feasible: Looking at 'mark_tbl' for instance, this is
> > > > accessed from callbacks in 'mark_type', so we would have to make nft_ctx
> > > > accessible by all functions dealing with datatypes.
> > > 
> > > Probably some specific new context object that wrap access to these
> > > tables, not necessarily nft_ctx.
> > > 
> > > It's just more code, it will not be a small patch, but I don't see any
> > > reason this can't be done.
> > 
> > Yes, this is my guess as well. Though for what benefit? I don't think
> > having this logic for global run-time data is bad per se. What is it
> > that you don't like about it?
> 
> This is breaking the assumption that releasing the context object will
> be clearing all state behind us.

Hmm. Does that matter here? I don't think it makes sense to make the
generated iproute2 tables context-local data. And I don't even see a way
to make gmp_init() and xt_init() apply per context only.

The effort of implementing this is quite high as well, since the table
pointers would have to be passed down to all places where datatypes are
parsed or printed.

Cheers, Phil