Re: [PATCH nf-next 1/3] netfilter: nf_tables: add generation mask to table objects

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Aug 04, 2015 at 12:26:35PM +0200, Patrick McHardy wrote:
> On 04.08, Pablo Neira Ayuso wrote:
> > On Tue, Aug 04, 2015 at 11:09:17AM +0200, Patrick McHardy wrote:
[...]
> > > I have a similar patch queued up, however there seems to be something missing
> > > in this patch. The lookup functions need to take the genmask into account.
> > 
> > They already do for the deletion case, so we hit -ENOENT for objects
> > that has been deleted in this batch, so we cannot delete objects
> > twice.
> 
> > @@ -829,10 +860,10 @@ static int nf_tables_deltable(struct sock *nlsk, struct sk_buff *skb,
> >       if (IS_ERR(afi))
> >               return PTR_ERR(afi);
> 
> > -	table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME]);
> > +	table = nf_tables_table_lookup(net, afi, nla[NFTA_TABLE_NAME], true);
> >       if (IS_ERR(table))
> >               return PTR_ERR(table);
> > -	if (table->flags & NFT_TABLE_INACTIVE)
> > +	if (!nft_table_is_active(net, table))
>                 return -ENOENT;
> 
> Looking at it, that part seems wrong. They need to be active in the *next*
> generation, not the current one, to be deleted. All netlink actions only
> affect the next generation.
> 
> The same bug is present in multiple locations.

That check is there to avoid the deletion of a table that has been
added in this batch, unlike the delete + add, the add + delete in the
same batch doesn't make much sense.

Revisiting this scenario, this how this looks if we remove that check:

preparation starts:

add: table X (10), added to table list (now inactive)
del: table X (11), inactive next.
              ^
              gencursor

commit starts (update gencursor):

add: table X (01): clear past and report event, *NOTE*: the rule table is inactive.
add: table X (01): delete from list and report event.
               ^
               gencursor

So it seems it should be fine to remove it as it is defensive. I think
robots can generate this kind of command placing updates in a batch,
anyway that should come in a follow up patch IMO.

> > > Otherwise you can not delete and add a new table in the same batch. The same
> > > holds for all other object types.
> > 
> > I can with this patch, we always operate with the *next* bit to
> > indicate that the object will be inactive in the future.
> > 
> > > > +static struct nft_table *nf_tables_table_lookup(struct net *net,
> > > > +						const struct nft_af_info *afi,
> > > > +						const struct nlattr *nla,
> > > > +						bool trans)
> > > >  {
> > > >  	struct nft_table *table;
> > > >  
> > > > @@ -382,10 +411,10 @@ static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
> > > >  		return ERR_PTR(-EINVAL);
> > > >  
> > > >  	table = nft_table_lookup(afi, nla);
> > > > -	if (table != NULL)
> > > > -		return table;
> > > > +	if (table == NULL || (trans && !nft_table_is_active_next(net, table)))
> > > > +		return ERR_PTR(-ENOENT);
> > > 
> > > We really need to check the genid itself, in some cases we *only* want
> > > currently active tables, f.i. gettable and dumps.
> > 
> > This is what this patch is doing from the dump path.
> > 
> > We shouldn't check if the object is active from the lookup function if
> > we're in the middle of a transaction, since we hold the lock there is
> > no way we can see inactive objects in the list. There's only one
> > transaction at the same time.
> 
> That's not entirely correct. Dump continuations happen asynchronously to
> netlink modifications and commit operations, so the genid may bump in the
> middle. We can get an inconsistent view if we have:
> 
> 			dump set elements from set x table y
> delete table y
> create table y
> create set x
> begin commit
> 			continue dump from new set

We catch this from the nfnlhdr->res_id field in the nfnetlink message,
but see below.

> commit, send NEWGEN
> 
> Sure, we will get a NEWGEN message, but at that time we might already have
> sent a full message for the new table/set since that message is only send
> after the commit is completed.

I agree in that an event message at the beginning of the commit phase
to announce the beginning new generation and another one to indicate
of this transaction.

- preparation phase -
delete table y
create table y
create set x
- commit phase -
send NEWGEN, attribute type: begin
delete table y
create table y
create set x
send NEWGEN, attribute type: end

Thanks for your feedback!