On Fri, Apr 27, 2018 at 12:25:25AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Apr 24, 2018 at 08:49:33PM +0200, Phil Sutter wrote:
> > Hi,
> >
> > On Tue, Apr 24, 2018 at 05:51:39PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Apr 20, 2018 at 09:21:12AM -0400, Eric Garver wrote:
> > > > On Fri, Apr 20, 2018 at 12:00:54PM +0200, Jan Engelhardt wrote:
> > > > >
> > > > > On Friday 2018-04-20 10:47, Pablo Neira Ayuso wrote:
> > > > > >> - if ((event != NFT_MSG_DELRULE) && (rule->list.prev !=
> > > > > >> &chain->rules)) {
> > > > > >> - prule = list_prev_entry(rule, list);
> > > > > >> - if (nla_put_be64(skb, NFTA_RULE_POSITION,
> > > > > >> - cpu_to_be64(prule->handle),
> > > > > >> + if ((event != NFT_MSG_DELRULE)) {
> > > > > >> + int pos = rule_get_position(rule, &chain->rules);
> > > > > >> +
> > > > > >> + if (nla_put_be64(skb, NFTA_RULE_POSITION,
> > > > > >> cpu_to_be64(pos),
> > > > > >
> > > > > >Rule counting to locate rule position is prone to races, the use of
> > > > > >the
> > > > > >handle to identify the position to replace rules help us make sure
> > > > > >that
> > > > > >we replace the right rule with several concurrent nft.
> > > > >
> > > > > That is absolutely understandable, but then the handle, which I take
> > > > > works like
> > > > > a uuid or an sql auto_increment, should be sent with a "handle"
> > > > > label, and not
> > > > > be advertised as a "position" or" index" IMO, since these two sort of
> > > > > convey a
> > > > > hole-free sequence.
> > > >
> > > > I think this is the real issue. IMO, "position" implies a counting
> > > > order.
> > > > Something like:
> > > >
> > > > "insert rule .. before <handle> .."
> > > > "add rule .. after <handle> .."
> > > >
> > > > would be much more intuitive.
> > >
> > > Yes, the before and after sounds more intuitive.
> > >
> > > Still, insert and add already comes with semantics (inherited from
> > > iptables) indicating before or after.
> > >
> > > Probably we can use 'at' instead of 'position'?
> > >
> > > insert rule ... at <handle>
> > > add rule ... at <handle>
> > >
> > > Although this still denotes position?
> >
> > How about using 'handle <handle>' for that? It would be clear what's
> > expected, and it is consistent with replace and delete commands.
> >
> > I still think allowing users to specify an absolute position to insert a
> > rule at would be nice to have, despite the potential problems that come
> > with it. Though I guess reusing 'position' keyword for it is not an
> > option due to backwards compatibility, right?
>
> I think the counter position to insert rule is fine if you need this
> for easy mapping with iptables.
>
> But I would recommend to use the handle, that's all, we can just
> document this.
>
> > My idea was to implement that absolute position support in userspace by
> > fetching the current ruleset and translating user-supplied position
> > index into a handle to pass to the kernel.
> >
> > What do you think?
>
> This is what iptables-compat is doing to support this. This is still
> prone to races, but this can be catches via ruleset generation
> validation (kernel sends EAGAIN to userspace if you request to delete
> a rule for a given generation) so it can transparently re-fetch the
> handle of the rule in that position and retry.
So probably you can add support for this in the kernel to make it
easier if you need this? I mean, I'm fine with this feature.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
