| Message ID | 20231019113347.8753-4-phil@nwl.cc |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce locking for rule reset requests | expand |
On Thu, Oct 19, 2023 at 01:33:47PM +0200, Phil Sutter wrote: > Rule reset is not concurrency-safe per-se, so multiple CPUs may reset > the same rule at the same time. At least counter and quota expressions > will suffer from value underruns in this case. > > Prevent this by introducing dedicated locking callbacks for nfnetlink > and the asynchronous dump handling to serialize access. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > Changes since v2: > - Keep local variable 'nft_net' in nf_tables_getrule_reset() > - No need for local variable 'family' in same function (used only once > after all the churn) > --- > net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++------- > 1 file changed, 60 insertions(+), 14 deletions(-) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 584d3b204372..fbb688c9903c 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c [...] > +static int nf_tables_dumpreset_rules(struct sk_buff *skb, > + struct netlink_callback *cb) > +{ > + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk)); > + int ret; > + > + mutex_lock(&nft_net->commit_mutex); > + ret = nf_tables_dump_rules(skb, cb); > + mutex_unlock(&nft_net->commit_mutex); NACK. This just mitigates the problem we are discussing, when there is an interference with an ongoing transaction.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Thu, Oct 19, 2023 at 01:33:47PM +0200, Phil Sutter wrote: > > Rule reset is not concurrency-safe per-se, so multiple CPUs may reset > > the same rule at the same time. At least counter and quota expressions > > will suffer from value underruns in this case. > > > > Prevent this by introducing dedicated locking callbacks for nfnetlink > > and the asynchronous dump handling to serialize access. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > Changes since v2: > > - Keep local variable 'nft_net' in nf_tables_getrule_reset() > > - No need for local variable 'family' in same function (used only once > > after all the churn) > > --- > > net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++------- > > 1 file changed, 60 insertions(+), 14 deletions(-) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 584d3b204372..fbb688c9903c 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > [...] > > +static int nf_tables_dumpreset_rules(struct sk_buff *skb, > > + struct netlink_callback *cb) > > +{ > > + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk)); > > + int ret; > > + > > + mutex_lock(&nft_net->commit_mutex); > > + ret = nf_tables_dump_rules(skb, cb); > > + mutex_unlock(&nft_net->commit_mutex); > > NACK. > > This just mitigates the problem we are discussing, when there is an > interference with an ongoing transaction. It resolves corrupting the internal state when two parallel resets are done. If you believe that we have to make entire dump consistent even when reset flag is given I see no choice but to completely remove reset-from-dump support. What is you suggested solution? AFAICS, with this series, userspace can, in theory, merge partial dumps into consistent output by manually collecting the partial dumps. That said, I think its not very realistic that userspace will get this right. That leaves: userspace does a dump (without reset), and if that was consistent walk it and do a per-handle get-with-reset request for each rule, then update the (not-yet-printed) dump with the newly obtained stateful results.
On Thu, Oct 19, 2023 at 01:38:04PM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 19, 2023 at 01:33:47PM +0200, Phil Sutter wrote: > > Rule reset is not concurrency-safe per-se, so multiple CPUs may reset > > the same rule at the same time. At least counter and quota expressions > > will suffer from value underruns in this case. > > > > Prevent this by introducing dedicated locking callbacks for nfnetlink > > and the asynchronous dump handling to serialize access. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > Changes since v2: > > - Keep local variable 'nft_net' in nf_tables_getrule_reset() > > - No need for local variable 'family' in same function (used only once > > after all the churn) > > --- > > net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++------- > > 1 file changed, 60 insertions(+), 14 deletions(-) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 584d3b204372..fbb688c9903c 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > [...] > > +static int nf_tables_dumpreset_rules(struct sk_buff *skb, > > + struct netlink_callback *cb) > > +{ > > + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk)); > > + int ret; > > + > > + mutex_lock(&nft_net->commit_mutex); > > + ret = nf_tables_dump_rules(skb, cb); > > + mutex_unlock(&nft_net->commit_mutex); > > NACK. > > This just mitigates the problem we are discussing, when there is an > interference with an ongoing transaction. This fixes for user space's ability to underrun counters and quotas because expressions' dump callbacks are not concurrency safe in reset mode. What you're concerned with is a different issue.
On Thu, Oct 19, 2023 at 02:33:50PM +0200, Phil Sutter wrote: > On Thu, Oct 19, 2023 at 01:38:04PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 19, 2023 at 01:33:47PM +0200, Phil Sutter wrote: > > > Rule reset is not concurrency-safe per-se, so multiple CPUs may reset > > > the same rule at the same time. At least counter and quota expressions > > > will suffer from value underruns in this case. > > > > > > Prevent this by introducing dedicated locking callbacks for nfnetlink > > > and the asynchronous dump handling to serialize access. > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > --- > > > Changes since v2: > > > - Keep local variable 'nft_net' in nf_tables_getrule_reset() > > > - No need for local variable 'family' in same function (used only once > > > after all the churn) > > > --- > > > net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++------- > > > 1 file changed, 60 insertions(+), 14 deletions(-) > > > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > > index 584d3b204372..fbb688c9903c 100644 > > > --- a/net/netfilter/nf_tables_api.c > > > +++ b/net/netfilter/nf_tables_api.c > > [...] > > > +static int nf_tables_dumpreset_rules(struct sk_buff *skb, > > > + struct netlink_callback *cb) > > > +{ > > > + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk)); > > > + int ret; > > > + > > > + mutex_lock(&nft_net->commit_mutex); > > > + ret = nf_tables_dump_rules(skb, cb); > > > + mutex_unlock(&nft_net->commit_mutex); > > > > NACK. > > > > This just mitigates the problem we are discussing, when there is an > > interference with an ongoing transaction. > > This fixes for user space's ability to underrun counters and quotas > because expressions' dump callbacks are not concurrency safe in reset > mode. > > What you're concerned with is a different issue. I'd suggest you add comment to this code, feel free to add better wording: /* Mutex is held is to prevent that two concurrent dump-and-reset calls * do not underrun counters and quotas. The commit_mutex is used for the * lack a better lock, this is not transaction path. */ mutex_lock(&nft_net->commit_mutex); ret = nf_tables_dump_rules(skb, cb); mutex_unlock(&nft_net->commit_mutex);
On Thu, Oct 19, 2023 at 01:59:09PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Thu, Oct 19, 2023 at 01:33:47PM +0200, Phil Sutter wrote: > > > Rule reset is not concurrency-safe per-se, so multiple CPUs may reset > > > the same rule at the same time. At least counter and quota expressions > > > will suffer from value underruns in this case. > > > > > > Prevent this by introducing dedicated locking callbacks for nfnetlink > > > and the asynchronous dump handling to serialize access. > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > --- > > > Changes since v2: > > > - Keep local variable 'nft_net' in nf_tables_getrule_reset() > > > - No need for local variable 'family' in same function (used only once > > > after all the churn) > > > --- > > > net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++------- > > > 1 file changed, 60 insertions(+), 14 deletions(-) > > > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > > index 584d3b204372..fbb688c9903c 100644 > > > --- a/net/netfilter/nf_tables_api.c > > > +++ b/net/netfilter/nf_tables_api.c > > [...] > > > +static int nf_tables_dumpreset_rules(struct sk_buff *skb, > > > + struct netlink_callback *cb) > > > +{ > > > + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk)); > > > + int ret; > > > + > > > + mutex_lock(&nft_net->commit_mutex); > > > + ret = nf_tables_dump_rules(skb, cb); > > > + mutex_unlock(&nft_net->commit_mutex); > > > > NACK. > > > > This just mitigates the problem we are discussing, when there is an > > interference with an ongoing transaction. > > It resolves corrupting the internal state when two parallel resets > are done. > > If you believe that we have to make entire dump consistent even > when reset flag is given I see no choice but to completely remove > reset-from-dump support. It is just the commit_mutex in this section that I don't like, but OK this operation and scenario should be rare and adding yet another lock for a rare case might make no sense. > What is you suggested solution? > > AFAICS, with this series, userspace can, in theory, merge partial > dumps into consistent output by manually collecting the partial > dumps. > > That said, I think its not very realistic that userspace will > get this right. I think this is possible, I would like to see userspace code (just simple sketch) that shows how complicate this is. > That leaves: userspace does a dump (without reset), and if that > was consistent walk it and do a per-handle get-with-reset request > for each rule, then update the (not-yet-printed) dump with the > newly obtained stateful results. Counter expressions in rules are probably the more complicated to restore. Named objects should be rather easy. Another possibility is to explore transaction control plane path and use NLM_F_ECHO to get back the original counters, but I would not really follow this path. I have been considering netlink dump based on ruleset snapshots. The idea is: Make a snapshot of the ruleset at the beginning of the netlink dump, this requires a new class of objects that can be used as container to store what is going to be. The snapshot should happen right before initiating the netlink dump holding the commit_mutex, and it would take memory to store this temporary ruleset snapshot. The snapshot would be done per dump request. This ensures no EINTR can happen, the ruleset at the end of the netlink dump will be consistent, but it might be slightly behind the real rule if updates have happened, but using the generation ID, it should be possible to at least report it to the user.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 584d3b204372..fbb688c9903c 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3551,6 +3551,19 @@ static int nf_tables_dump_rules(struct sk_buff *skb, return skb->len; } +static int nf_tables_dumpreset_rules(struct sk_buff *skb, + struct netlink_callback *cb) +{ + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk)); + int ret; + + mutex_lock(&nft_net->commit_mutex); + ret = nf_tables_dump_rules(skb, cb); + mutex_unlock(&nft_net->commit_mutex); + + return ret; +} + static int nf_tables_dump_rules_start(struct netlink_callback *cb) { struct nft_rule_dump_ctx *ctx = (void *)cb->ctx; @@ -3570,12 +3583,18 @@ static int nf_tables_dump_rules_start(struct netlink_callback *cb) return -ENOMEM; } } - if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET) - ctx->reset = true; - return 0; } +static int nf_tables_dumpreset_rules_start(struct netlink_callback *cb) +{ + struct nft_rule_dump_ctx *ctx = (void *)cb->ctx; + + ctx->reset = true; + + return nf_tables_dump_rules_start(cb); +} + static int nf_tables_dump_rules_done(struct netlink_callback *cb) { struct nft_rule_dump_ctx *ctx = (void *)cb->ctx; @@ -3636,13 +3655,9 @@ nf_tables_getrule_single(u32 portid, const struct nfnl_info *info, static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info, const struct nlattr * const nla[]) { - struct nftables_pernet *nft_net = nft_pernet(info->net); u32 portid = NETLINK_CB(skb).portid; struct net *net = info->net; struct sk_buff *skb2; - bool reset = false; - char *tablename; - char *buf; if (info->nlh->nlmsg_flags & NLM_F_DUMP) { struct netlink_dump_control c = { @@ -3656,15 +3671,46 @@ static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info, return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c); } - if (NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET) - reset = true; - - skb2 = nf_tables_getrule_single(portid, info, nla, reset); + skb2 = nf_tables_getrule_single(portid, info, nla, false); if (IS_ERR(skb2)) return PTR_ERR(skb2); - if (!reset) - return nfnetlink_unicast(skb2, net, portid); + return nfnetlink_unicast(skb2, net, portid); +} + +static int nf_tables_getrule_reset(struct sk_buff *skb, + const struct nfnl_info *info, + const struct nlattr * const nla[]) +{ + struct nftables_pernet *nft_net = nft_pernet(info->net); + u32 portid = NETLINK_CB(skb).portid; + struct net *net = info->net; + char *tablename, *buf; + struct sk_buff *skb2; + + if (info->nlh->nlmsg_flags & NLM_F_DUMP) { + struct netlink_dump_control c = { + .start= nf_tables_dumpreset_rules_start, + .dump = nf_tables_dumpreset_rules, + .done = nf_tables_dump_rules_done, + .module = THIS_MODULE, + .data = (void *)nla, + }; + + return nft_netlink_dump_start_rcu(info->sk, skb, info->nlh, &c); + } + + if (!try_module_get(THIS_MODULE)) + return -EINVAL; + rcu_read_unlock(); + mutex_lock(&nft_net->commit_mutex); + skb2 = nf_tables_getrule_single(portid, info, nla, true); + mutex_unlock(&nft_net->commit_mutex); + rcu_read_lock(); + module_put(THIS_MODULE); + + if (IS_ERR(skb2)) + return PTR_ERR(skb2); tablename = nla_strdup(nla[NFTA_RULE_TABLE], GFP_ATOMIC); buf = kasprintf(GFP_ATOMIC, "%s:%u", tablename, nft_net->base_seq); @@ -9003,7 +9049,7 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = { .policy = nft_rule_policy, }, [NFT_MSG_GETRULE_RESET] = { - .call = nf_tables_getrule, + .call = nf_tables_getrule_reset, .type = NFNL_CB_RCU, .attr_count = NFTA_RULE_MAX, .policy = nft_rule_policy,
Rule reset is not concurrency-safe per-se, so multiple CPUs may reset the same rule at the same time. At least counter and quota expressions will suffer from value underruns in this case. Prevent this by introducing dedicated locking callbacks for nfnetlink and the asynchronous dump handling to serialize access. Signed-off-by: Phil Sutter <phil@nwl.cc> --- Changes since v2: - Keep local variable 'nft_net' in nf_tables_getrule_reset() - No need for local variable 'family' in same function (used only once after all the churn) --- net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 14 deletions(-)