| Message ID | 1392377228-3748-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Deferred |
| Headers | show |
[ sorry accidentally dropped netfilter-devel ] On Fri, Feb 14, 2014 at 12:27:08PM +0100, Pablo Neira Ayuso wrote: > If some modules are missing while processing a rule batch, the updates > are aborted to start scratch since the nfnl lock was released. If the > rule-set contains this configuration (in this order): > > #1 rule using anonymous set > #2 rule requiring module autoload > > The anonymous set will be released when aborting. This patch fixes this > by passing a context variable (autoload) that can be used to decide if > the anonymous set has to be released or not. > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > I guess we can encapsulate that autoload into a context information structure > in the future in case any other information is needed in the rule destroy path > to make this look nicer. > > I started hacking on two patches to net-next, one to include table, chains and > set into the batch and follow up to add atomic updates for sets. @Patrick: I > think that should not interfer with your set enhancements. Wouldn't be a big problem, they're pretty much contained to newset(). Regarding this patch - I'd really prefer to just fix batches to include sets instead of changing all these function signatures just to handle this very specific case. I'm wondering how this will work in case of anonymous sets though, right now we need two transactions so userspace can attach the new set to the lookup expression. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 11:37:56AM +0000, Patrick McHardy wrote: > [ sorry accidentally dropped netfilter-devel ] > > On Fri, Feb 14, 2014 at 12:27:08PM +0100, Pablo Neira Ayuso wrote: > > If some modules are missing while processing a rule batch, the updates > > are aborted to start scratch since the nfnl lock was released. If the > > rule-set contains this configuration (in this order): > > > > #1 rule using anonymous set > > #2 rule requiring module autoload > > > > The anonymous set will be released when aborting. This patch fixes this > > by passing a context variable (autoload) that can be used to decide if > > the anonymous set has to be released or not. > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > I guess we can encapsulate that autoload into a context information structure > > in the future in case any other information is needed in the rule destroy path > > to make this look nicer. > > > > I started hacking on two patches to net-next, one to include table, chains and > > set into the batch and follow up to add atomic updates for sets. @Patrick: I > > think that should not interfer with your set enhancements. > > Wouldn't be a big problem, they're pretty much contained to newset(). > > Regarding this patch - I'd really prefer to just fix batches to include sets > instead of changing all these function signatures just to handle this very > specific case. If the patch that results from adding the set into the batch support is ~100 LOC, we can pass that to -stable, but if it doesn't, we'll have to pass this first or tell people that they need to load all modules as a workaround. > I'm wondering how this will work in case of anonymous sets though, right now > we need two transactions so userspace can attach the new set to the lookup > expression. The set definition and the elements need to be included in the lookup expression for anonymous sets, can you think of any better solution? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 01:34:11PM +0100, Pablo Neira Ayuso wrote: > On Fri, Feb 14, 2014 at 11:37:56AM +0000, Patrick McHardy wrote: > > [ sorry accidentally dropped netfilter-devel ] > > > > On Fri, Feb 14, 2014 at 12:27:08PM +0100, Pablo Neira Ayuso wrote: > > > If some modules are missing while processing a rule batch, the updates > > > are aborted to start scratch since the nfnl lock was released. If the > > > rule-set contains this configuration (in this order): > > > > > > #1 rule using anonymous set > > > #2 rule requiring module autoload > > > > > > The anonymous set will be released when aborting. This patch fixes this > > > by passing a context variable (autoload) that can be used to decide if > > > the anonymous set has to be released or not. > > > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > --- > > > I guess we can encapsulate that autoload into a context information structure > > > in the future in case any other information is needed in the rule destroy path > > > to make this look nicer. > > > > > > I started hacking on two patches to net-next, one to include table, chains and > > > set into the batch and follow up to add atomic updates for sets. @Patrick: I > > > think that should not interfer with your set enhancements. > > > > Wouldn't be a big problem, they're pretty much contained to newset(). > > > > Regarding this patch - I'd really prefer to just fix batches to include sets > > instead of changing all these function signatures just to handle this very > > specific case. > > If the patch that results from adding the set into the batch support > is ~100 LOC, we can pass that to -stable, but if it doesn't, we'll > have to pass this first or tell people that they need to load all > modules as a workaround. I guess we don't necessarily would have to pass it to -stable since its not a regression. > > I'm wondering how this will work in case of anonymous sets though, right now > > we need two transactions so userspace can attach the new set to the lookup > > expression. > > The set definition and the elements need to be included in the lookup > expression for anonymous sets, can you think of any better solution? I think we can use some identifiers generated by userspace to tie them both together. Something like a unique numeric identifier (unique within the transaction). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 15, 2014 at 09:38:23AM +0000, Patrick McHardy wrote: > On Fri, Feb 14, 2014 at 01:34:11PM +0100, Pablo Neira Ayuso wrote: > > On Fri, Feb 14, 2014 at 11:37:56AM +0000, Patrick McHardy wrote: > > > [ sorry accidentally dropped netfilter-devel ] > > > > > > On Fri, Feb 14, 2014 at 12:27:08PM +0100, Pablo Neira Ayuso wrote: > > > > If some modules are missing while processing a rule batch, the updates > > > > are aborted to start scratch since the nfnl lock was released. If the > > > > rule-set contains this configuration (in this order): > > > > > > > > #1 rule using anonymous set > > > > #2 rule requiring module autoload > > > > > > > > The anonymous set will be released when aborting. This patch fixes this > > > > by passing a context variable (autoload) that can be used to decide if > > > > the anonymous set has to be released or not. > > > > > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > --- > > > > I guess we can encapsulate that autoload into a context information structure > > > > in the future in case any other information is needed in the rule destroy path > > > > to make this look nicer. > > > > > > > > I started hacking on two patches to net-next, one to include table, chains and > > > > set into the batch and follow up to add atomic updates for sets. @Patrick: I > > > > think that should not interfer with your set enhancements. > > > > > > Wouldn't be a big problem, they're pretty much contained to newset(). > > > > > > Regarding this patch - I'd really prefer to just fix batches to include sets > > > instead of changing all these function signatures just to handle this very > > > specific case. > > > > If the patch that results from adding the set into the batch support > > is ~100 LOC, we can pass that to -stable, but if it doesn't, we'll > > have to pass this first or tell people that they need to load all > > modules as a workaround. > > I guess we don't necessarily would have to pass it to -stable since its > not a regression. > > > > I'm wondering how this will work in case of anonymous sets though, right now > > > we need two transactions so userspace can attach the new set to the lookup > > > expression. > > > > The set definition and the elements need to be included in the lookup > > expression for anonymous sets, can you think of any better solution? > > I think we can use some identifiers generated by userspace to tie them > both together. Something like a unique numeric identifier (unique within > the transaction). That can be done, but I don't see why we allow the creation of anonymous sets out of the scope of a rule since: * They can only be used by one single rule. * You cannot update them by adding/deleting elements. The current API allows creating an anonymous set that can be left unused. I think we should only allow the creation of non-anonymous sets via NFT_MSG_NEWSET at some point. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 16, 2014 at 11:33:35AM +0100, Pablo Neira Ayuso wrote: > On Sat, Feb 15, 2014 at 09:38:23AM +0000, Patrick McHardy wrote: > > > > > > The set definition and the elements need to be included in the lookup > > > expression for anonymous sets, can you think of any better solution? > > > > I think we can use some identifiers generated by userspace to tie them > > both together. Something like a unique numeric identifier (unique within > > the transaction). > > That can be done, but I don't see why we allow the creation of > anonymous sets out of the scope of a rule since: > > * They can only be used by one single rule. > * You cannot update them by adding/deleting elements. > > The current API allows creating an anonymous set that can be left > unused. I think we should only allow the creation of non-anonymous > sets via NFT_MSG_NEWSET at some point. The two main reasons are: - it keeps the API simpler - members might not fit into a single message and currently we can keep adding members as long as the set is not bound I don't think we should change this. It actually also is possible to use anonymous sets with more than one rule, just nft doesn't provide a way to do it. The definition of an anonymous set it (anonymous isn't the best name) a set that is automatically destroyed once the last rule unbinds. The fact that we don't allow to use them in multiple rules is purely internal to nft. On a general note, nft is just meant to be *one* frontend, there's no reason why someone else couldn't write a different one more suitable for a specific purpose. F.i. a simple embedded system might only use tuples of (dst,proto,port) and use a hash for the lookup. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 16, 2014 at 10:44:09AM +0000, Patrick McHardy wrote: > On Sun, Feb 16, 2014 at 11:33:35AM +0100, Pablo Neira Ayuso wrote: > > On Sat, Feb 15, 2014 at 09:38:23AM +0000, Patrick McHardy wrote: > > > > > > > > The set definition and the elements need to be included in the lookup > > > > expression for anonymous sets, can you think of any better solution? > > > > > > I think we can use some identifiers generated by userspace to tie them > > > both together. Something like a unique numeric identifier (unique within > > > the transaction). > > > > That can be done, but I don't see why we allow the creation of > > anonymous sets out of the scope of a rule since: > > > > * They can only be used by one single rule. > > * You cannot update them by adding/deleting elements. > > > > The current API allows creating an anonymous set that can be left > > unused. I think we should only allow the creation of non-anonymous > > sets via NFT_MSG_NEWSET at some point. > > The two main reasons are: > > - it keeps the API simpler > - members might not fit into a single message and currently we can keep > adding members as long as the set is not bound I don't find a reason why not to use a non-anonymous (named) set in the "we need to add more elements" scenario. Going back to your proposal of using internal set ids to the transaction, we'll need to keep a temporary list of sets that has been created in that transaction, then once they are bound to the rule, we'll have to move the per-table set lists. > I don't think we should change this. > > It actually also is possible to use anonymous sets with more than one > rule, just nft doesn't provide a way to do it. The definition of an > anonymous set it (anonymous isn't the best name) a set that is automatically > destroyed once the last rule unbinds. I like the "release set when unused" feature, but I think we can add that to named sets at some point. We can just add a different flag so named sets also benefit from this feature. I'm seeing different features that can be combined: * Dynamically allocate the name, this is useful in case the user wants to avoid a clash when allocating the set name. * Release after set is unused, so the user doesn't need to release the set from its application, which can be good in the dynamic rule-set case. * Don't allow updates, so the user makes sure nobody from userspace can updates the content of that set anymore. So anonymous sets will stand for the combination in which those three are set. > The fact that we don't allow to use them in multiple rules is purely > internal to nft. > > On a general note, nft is just meant to be *one* frontend, there's no > reason why someone else couldn't write a different one more suitable for > a specific purpose. F.i. a simple embedded system might only use tuples > of (dst,proto,port) and use a hash for the lookup. Agreed, not only thinking of nft but on the interface that we'll expose to userspace. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 16, 2014 at 12:14:50PM +0100, Pablo Neira Ayuso wrote: > On Sun, Feb 16, 2014 at 10:44:09AM +0000, Patrick McHardy wrote: > > > > > > That can be done, but I don't see why we allow the creation of > > > anonymous sets out of the scope of a rule since: > > > > > > * They can only be used by one single rule. > > > * You cannot update them by adding/deleting elements. > > > > > > The current API allows creating an anonymous set that can be left > > > unused. I think we should only allow the creation of non-anonymous > > > sets via NFT_MSG_NEWSET at some point. > > > > The two main reasons are: > > > > - it keeps the API simpler > > - members might not fit into a single message and currently we can keep > > adding members as long as the set is not bound > > I don't find a reason why not to use a non-anonymous (named) set in > the "we need to add more elements" scenario. Its not that we add to add more elements later. Its simply that the amount of elements specified while creating the rule exceeds the possible size we can squash into a nested attribute. Simply: nft filter output ip daddr { 10000 addresses } => fail There's no reason to introduce this limitation. > Going back to your proposal of using internal set ids to the > transaction, we'll need to keep a temporary list of sets that has been > created in that transaction, then once they are bound to the rule, > we'll have to move the per-table set lists. Yes, similar to the expressions lists. But that seems to be independant of whether we use IDs or not, we simply have to be able to abort a failed transaction. > > I don't think we should change this. > > > > It actually also is possible to use anonymous sets with more than one > > rule, just nft doesn't provide a way to do it. The definition of an > > anonymous set it (anonymous isn't the best name) a set that is automatically > > destroyed once the last rule unbinds. > > I like the "release set when unused" feature, but I think we can add > that to named sets at some point. We can just add a different flag so > named sets also benefit from this feature. I'm seeing different > features that can be combined: > > * Dynamically allocate the name, this is useful in case the user > wants to avoid a clash when allocating the set name. This only works for literal sets though since we need a way to reference them. Speaking of nft of course. > * Release after set is unused, so the user doesn't need to release the > set from its application, which can be good in the dynamic rule-set > case. Might be useful, I agree. > * Don't allow updates, so the user makes sure nobody from userspace > can updates the content of that set anymore. This is already possible for non-anonymous sets by specifying the "constant" flag. I'll change that syntax though with the set descriptions. > So anonymous sets will stand for the combination in which those three > are set. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h index 28c7436..69febee 100644 --- a/include/linux/netfilter/nfnetlink.h +++ b/include/linux/netfilter/nfnetlink.h @@ -27,7 +27,7 @@ struct nfnetlink_subsystem { __u8 cb_count; /* number of callbacks */ const struct nfnl_callback *cb; /* callback for individual types */ int (*commit)(struct sk_buff *skb); - int (*abort)(struct sk_buff *skb); + int (*abort)(struct sk_buff *skb, bool autoload); }; int nfnetlink_subsys_register(const struct nfnetlink_subsystem *n); diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h index e7e14ff..ccc4be6 100644 --- a/include/net/netfilter/nf_tables.h +++ b/include/net/netfilter/nf_tables.h @@ -239,7 +239,7 @@ struct nft_set_binding { int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set, struct nft_set_binding *binding); void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set, - struct nft_set_binding *binding); + struct nft_set_binding *binding, bool autoload); /** @@ -288,7 +288,8 @@ struct nft_expr_ops { int (*init)(const struct nft_ctx *ctx, const struct nft_expr *expr, const struct nlattr * const tb[]); - void (*destroy)(const struct nft_expr *expr); + void (*destroy)(const struct nft_expr *expr, + bool autoload); int (*dump)(struct sk_buff *skb, const struct nft_expr *expr); int (*validate)(const struct nft_ctx *ctx, diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index adce01e..f92d99d 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -1254,10 +1254,10 @@ err1: return err; } -static void nf_tables_expr_destroy(struct nft_expr *expr) +static void nf_tables_expr_destroy(struct nft_expr *expr, bool autoload) { if (expr->ops->destroy) - expr->ops->destroy(expr); + expr->ops->destroy(expr, autoload); module_put(expr->ops->type->owner); } @@ -1531,7 +1531,7 @@ err: return err; } -static void nf_tables_rule_destroy(struct nft_rule *rule) +static void nf_tables_rule_destroy(struct nft_rule *rule, bool autoload) { struct nft_expr *expr; @@ -1541,7 +1541,7 @@ static void nf_tables_rule_destroy(struct nft_rule *rule) */ expr = nft_expr_first(rule); while (expr->ops && expr != nft_expr_last(rule)) { - nf_tables_expr_destroy(expr); + nf_tables_expr_destroy(expr, autoload); expr = nft_expr_next(expr); } kfree(rule); @@ -1709,7 +1709,7 @@ err3: kfree(repl); } err2: - nf_tables_rule_destroy(rule); + nf_tables_rule_destroy(rule, false); err1: for (i = 0; i < n; i++) { if (info[i].ops != NULL) @@ -1840,7 +1840,7 @@ static int nf_tables_commit(struct sk_buff *skb) /* Now we can safely release unused old rules */ list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) { - nf_tables_rule_destroy(rupd->rule); + nf_tables_rule_destroy(rupd->rule, false); list_del(&rupd->list); kfree(rupd); } @@ -1848,7 +1848,7 @@ static int nf_tables_commit(struct sk_buff *skb) return 0; } -static int nf_tables_abort(struct sk_buff *skb) +static int nf_tables_abort(struct sk_buff *skb, bool autoload) { struct net *net = sock_net(skb->sk); struct nft_rule_trans *rupd, *tmp; @@ -1869,7 +1869,7 @@ static int nf_tables_abort(struct sk_buff *skb) synchronize_rcu(); list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) { - nf_tables_rule_destroy(rupd->rule); + nf_tables_rule_destroy(rupd->rule, autoload); list_del(&rupd->list); kfree(rupd); } @@ -2518,11 +2518,12 @@ bind: } void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set, - struct nft_set_binding *binding) + struct nft_set_binding *binding, bool autoload) { list_del(&binding->list); - if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS) + if (list_empty(&set->bindings) && + set->flags & NFT_SET_ANONYMOUS && !autoload) nf_tables_set_destroy(ctx, set); } diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c index 046aa13..61fb987 100644 --- a/net/netfilter/nfnetlink.c +++ b/net/netfilter/nfnetlink.c @@ -325,7 +325,7 @@ replay: * original skb. */ if (err == -EAGAIN) { - ss->abort(skb); + ss->abort(skb, true); nfnl_unlock(subsys_id); kfree_skb(nskb); goto replay; @@ -351,7 +351,7 @@ done: if (success && done) ss->commit(skb); else - ss->abort(skb); + ss->abort(skb, false); nfnl_unlock(subsys_id); kfree_skb(nskb); diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c index 82cb823..d1e02d9 100644 --- a/net/netfilter/nft_compat.c +++ b/net/netfilter/nft_compat.c @@ -192,7 +192,7 @@ err: } static void -nft_target_destroy(const struct nft_expr *expr) +nft_target_destroy(const struct nft_expr *expr, bool autoload) { struct xt_target *target = expr->ops->data; @@ -379,7 +379,7 @@ err: } static void -nft_match_destroy(const struct nft_expr *expr) +nft_match_destroy(const struct nft_expr *expr, bool autoload) { struct xt_match *match = expr->ops->data; diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c index 46e2754..178d2be 100644 --- a/net/netfilter/nft_ct.c +++ b/net/netfilter/nft_ct.c @@ -297,7 +297,7 @@ static int nft_ct_init(const struct nft_ctx *ctx, return 0; } -static void nft_ct_destroy(const struct nft_expr *expr) +static void nft_ct_destroy(const struct nft_expr *expr, bool autoload) { struct nft_ct *priv = nft_expr_priv(expr); diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c index f169501..961042f 100644 --- a/net/netfilter/nft_immediate.c +++ b/net/netfilter/nft_immediate.c @@ -70,7 +70,7 @@ err1: return err; } -static void nft_immediate_destroy(const struct nft_expr *expr) +static void nft_immediate_destroy(const struct nft_expr *expr, bool autoload) { const struct nft_immediate_expr *priv = nft_expr_priv(expr); return nft_data_uninit(&priv->data, nft_dreg_to_type(priv->dreg)); diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c index 26c5154..b018eba 100644 --- a/net/netfilter/nft_log.c +++ b/net/netfilter/nft_log.c @@ -74,7 +74,7 @@ static int nft_log_init(const struct nft_ctx *ctx, return 0; } -static void nft_log_destroy(const struct nft_expr *expr) +static void nft_log_destroy(const struct nft_expr *expr, bool autoload) { struct nft_log *priv = nft_expr_priv(expr); diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c index bb4ef4c..c2b30c2 100644 --- a/net/netfilter/nft_lookup.c +++ b/net/netfilter/nft_lookup.c @@ -89,11 +89,11 @@ static int nft_lookup_init(const struct nft_ctx *ctx, return 0; } -static void nft_lookup_destroy(const struct nft_expr *expr) +static void nft_lookup_destroy(const struct nft_expr *expr, bool autoload) { struct nft_lookup *priv = nft_expr_priv(expr); - nf_tables_unbind_set(NULL, priv->set, &priv->binding); + nf_tables_unbind_set(NULL, priv->set, &priv->binding, autoload); } static int nft_lookup_dump(struct sk_buff *skb, const struct nft_expr *expr)
If some modules are missing while processing a rule batch, the updates are aborted to start scratch since the nfnl lock was released. If the rule-set contains this configuration (in this order): #1 rule using anonymous set #2 rule requiring module autoload The anonymous set will be released when aborting. This patch fixes this by passing a context variable (autoload) that can be used to decide if the anonymous set has to be released or not. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- I guess we can encapsulate that autoload into a context information structure in the future in case any other information is needed in the rule destroy path to make this look nicer. I started hacking on two patches to net-next, one to include table, chains and set into the batch and follow up to add atomic updates for sets. @Patrick: I think that should not interfer with your set enhancements. include/linux/netfilter/nfnetlink.h | 2 +- include/net/netfilter/nf_tables.h | 5 +++-- net/netfilter/nf_tables_api.c | 21 +++++++++++---------- net/netfilter/nfnetlink.c | 4 ++-- net/netfilter/nft_compat.c | 4 ++-- net/netfilter/nft_ct.c | 2 +- net/netfilter/nft_immediate.c | 2 +- net/netfilter/nft_log.c | 2 +- net/netfilter/nft_lookup.c | 4 ++-- 9 files changed, 24 insertions(+), 22 deletions(-)