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?
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?
Cheers, Phil
--
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
