On Wed, Oct 25, 2023 at 10:08:28PM +0200, Phil Sutter wrote:
> Objects' dump callbacks are not concurrency-safe per-se with reset bit
> set. If two CPUs perform a reset at the same time, at least counter and
> quota objects suffer from value underrun.
>
> Prevent this by introducing dedicated locking callbacks for nfnetlink
> and the asynchronous dump handling to serialize access.
>
> Signed-off-by: Phil Sutter <phil@xxxxxx>
> ---
> net/netfilter/nf_tables_api.c | 72 ++++++++++++++++++++++++++++-------
> 1 file changed, 59 insertions(+), 13 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 5f84bdd40c3f..245a2c5be082 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
[...]
> @@ -7832,16 +7876,18 @@ static int nf_tables_getobj(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_GETOBJ_RESET)
> - reset = true;
> + if (!try_module_get(THIS_MODULE))
> + return -EINVAL;
For netlink dump path, __netlink_dump_start() already grabs a
reference module this via c->module.
Why is this module reference needed for getting one object? This does
not follow netlink dump path, it creates the skb and it returns
inmediately.
> + rcu_read_unlock();
> + mutex_lock(&nft_net->commit_mutex);
> + skb2 = nf_tables_getobj_single(portid, info, nla, true);
> + mutex_unlock(&nft_net->commit_mutex);
> + rcu_read_lock();
> + module_put(THIS_MODULE);
>
> - skb2 = nf_tables_getobj_single(portid, info, nla, reset);
> if (IS_ERR(skb2))
> return PTR_ERR(skb2);
>
> - if (!reset)
> - return nfnetlink_unicast(skb2, net, NETLINK_CB(skb).portid);
This is what gets added in 1/3 that goes away, I see.
> -
> buf = kasprintf(GFP_ATOMIC, "%.*s:%u",
> nla_len(nla[NFTA_OBJ_TABLE]),
> (char *)nla_data(nla[NFTA_OBJ_TABLE]),
> @@ -9128,7 +9174,7 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
> .policy = nft_obj_policy,
> },
> [NFT_MSG_GETOBJ_RESET] = {
> - .call = nf_tables_getobj,
> + .call = nf_tables_getobj_reset,
> .type = NFNL_CB_RCU,
> .attr_count = NFTA_OBJ_MAX,
> .policy = nft_obj_policy,
> --
> 2.41.0
>
