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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
