From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Patrick McHardy <kaber@trash.net>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH nf-next 1/3] netfilter: nf_tables: add generation mask to table objects
Date: Thu, 6 Aug 2015 12:20:43 +0200 [thread overview]
Message-ID: <20150806102043.GA18683@salvia> (raw)
In-Reply-To: <20150805090915.GD13187@acer.localdomain>
On Wed, Aug 05, 2015 at 11:09:16AM +0200, Patrick McHardy wrote:
> On 04.08, Pablo Neira Ayuso wrote:
[...]
> > 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.
>
> I don't follow. Why add an unnecessary check just to remove it again?
> As I said, the only thing that matters is the next generation, we should
> never even look at the current one when performing actions.
Yes, we can remove those checks to reject add+del in the same batch in
first place.
I remember I added this because I found some problematic scenario, but
given looking at the example above, I agree we can remove this first
place. I'm going to recheck for other objects too.
> > > > 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!
>
> That might work if the message ordering is then guaranteed. However I think
> we can fix this case without changing NEWGEN. Let me think about that a bit,
> for now just taking care of the genid checks correctly seems like a good
> step forward.
But we can catch this problem through ->res_id, OK?
> BTW, we also need to adjust loop detection to only take into account
> active rules, active chains, active sets etc.
Indeed, thanks Patrick.
Will you take care of this? It would be great to have a fix for these
in this merge window. On top of that, I have a patchset here to add
named expressions as you suggested as a generic way to implement named
counters (or any other stateful expression) and I need that this is
fixed first so I don't need to add another ugly _INACTIVE flag to the
nft_nexpr object.
Let me know, thanks!
next prev parent reply other threads:[~2015-08-06 10:14 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-04 9:05 [PATCH nf-next 1/3] netfilter: nf_tables: add generation mask to table objects Pablo Neira Ayuso
2015-08-04 9:05 ` [PATCH nf-next 2/3] netfilter: nf_tables: add generation mask to chain objects Pablo Neira Ayuso
2015-08-04 9:05 ` [PATCH nf-next 3/3] netfilter: nf_tables: add generation mask to set objects Pablo Neira Ayuso
2015-08-04 9:12 ` Patrick McHardy
2015-08-04 9:29 ` Pablo Neira Ayuso
2015-08-04 10:38 ` Patrick McHardy
2015-08-04 17:23 ` Pablo Neira Ayuso
2015-08-04 9:09 ` [PATCH nf-next 1/3] netfilter: nf_tables: add generation mask to table objects Patrick McHardy
2015-08-04 9:29 ` Pablo Neira Ayuso
2015-08-04 10:26 ` Patrick McHardy
2015-08-04 17:04 ` Pablo Neira Ayuso
2015-08-05 9:09 ` Patrick McHardy
2015-08-06 10:20 ` Pablo Neira Ayuso [this message]
2015-08-08 15:53 ` Patrick McHardy
2015-08-10 7:56 ` Patrick McHardy
2015-08-10 18:37 ` Pablo Neira Ayuso
2015-08-04 18:21 ` Pablo Neira Ayuso
2015-08-05 8:41 ` Patrick McHardy
2015-08-06 10:21 ` Pablo Neira Ayuso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150806102043.GA18683@salvia \
--to=pablo@netfilter.org \
--cc=kaber@trash.net \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).