Hello,
On Mon, 16 Oct 2023, xiaolinkui wrote:
> From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
>
> The ref_netlink appears to solve the swap race problem. In addition to the
> netlink events, there are other factors that trigger this race condition.
> The race condition in the ip_set_test will be fixed in the next patch.
Sorry, I do not accept this patch. It adds nothing to the code, it's just
renaming. Additionally it renames a generic, netlink-centric object to a
specific one which just confuses the reader of the code.
Best regards,
Jozsef
> Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> ---
> include/linux/netfilter/ipset/ip_set.h | 4 +--
> net/netfilter/ipset/ip_set_core.c | 34 +++++++++++++-------------
> 2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> index e8c350a3ade1..32c56f1a43f2 100644
> --- a/include/linux/netfilter/ipset/ip_set.h
> +++ b/include/linux/netfilter/ipset/ip_set.h
> @@ -248,10 +248,10 @@ struct ip_set {
> spinlock_t lock;
> /* References to the set */
> u32 ref;
> - /* References to the set for netlink events like dump,
> + /* References to the set for netlink/test events,
> * ref can be swapped out by ip_set_swap
> */
> - u32 ref_netlink;
> + u32 ref_swapping;
> /* The core set type */
> struct ip_set_type *type;
> /* The type variant doing the real job */
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 35d2f9c9ada0..e5d25df5c64c 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -59,7 +59,7 @@ MODULE_ALIAS_NFNL_SUBSYS(NFNL_SUBSYS_IPSET);
> lockdep_is_held(&ip_set_ref_lock))
> #define ip_set(inst, id) \
> ip_set_dereference((inst)->ip_set_list)[id]
> -#define ip_set_ref_netlink(inst,id) \
> +#define ip_set_ref_swapping(inst, id) \
> rcu_dereference_raw((inst)->ip_set_list)[id]
>
> /* The set types are implemented in modules and registered set types
> @@ -683,19 +683,19 @@ __ip_set_put(struct ip_set *set)
> * a separate reference counter
> */
> static void
> -__ip_set_get_netlink(struct ip_set *set)
> +__ip_set_get_swapping(struct ip_set *set)
> {
> write_lock_bh(&ip_set_ref_lock);
> - set->ref_netlink++;
> + set->ref_swapping++;
> write_unlock_bh(&ip_set_ref_lock);
> }
>
> static void
> -__ip_set_put_netlink(struct ip_set *set)
> +__ip_set_put_swapping(struct ip_set *set)
> {
> write_lock_bh(&ip_set_ref_lock);
> - BUG_ON(set->ref_netlink == 0);
> - set->ref_netlink--;
> + BUG_ON(set->ref_swapping == 0);
> + set->ref_swapping--;
> write_unlock_bh(&ip_set_ref_lock);
> }
>
> @@ -1213,7 +1213,7 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
> if (!attr[IPSET_ATTR_SETNAME]) {
> for (i = 0; i < inst->ip_set_max; i++) {
> s = ip_set(inst, i);
> - if (s && (s->ref || s->ref_netlink)) {
> + if (s && (s->ref || s->ref_swapping)) {
> ret = -IPSET_ERR_BUSY;
> goto out;
> }
> @@ -1237,7 +1237,7 @@ static int ip_set_destroy(struct sk_buff *skb, const struct nfnl_info *info,
> if (!(flags & IPSET_FLAG_EXIST))
> ret = -ENOENT;
> goto out;
> - } else if (s->ref || s->ref_netlink) {
> + } else if (s->ref || s->ref_swapping) {
> ret = -IPSET_ERR_BUSY;
> goto out;
> }
> @@ -1321,7 +1321,7 @@ static int ip_set_rename(struct sk_buff *skb, const struct nfnl_info *info,
> return -ENOENT;
>
> write_lock_bh(&ip_set_ref_lock);
> - if (set->ref != 0 || set->ref_netlink != 0) {
> + if (set->ref != 0 || set->ref_swapping != 0) {
> ret = -IPSET_ERR_REFERENCED;
> goto out;
> }
> @@ -1383,7 +1383,7 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info,
>
> write_lock_bh(&ip_set_ref_lock);
>
> - if (from->ref_netlink || to->ref_netlink) {
> + if (from->ref_swapping || to->ref_swapping) {
> write_unlock_bh(&ip_set_ref_lock);
> return -EBUSY;
> }
> @@ -1441,12 +1441,12 @@ ip_set_dump_done(struct netlink_callback *cb)
> struct ip_set_net *inst =
> (struct ip_set_net *)cb->args[IPSET_CB_NET];
> ip_set_id_t index = (ip_set_id_t)cb->args[IPSET_CB_INDEX];
> - struct ip_set *set = ip_set_ref_netlink(inst, index);
> + struct ip_set *set = ip_set_ref_swapping(inst, index);
>
> if (set->variant->uref)
> set->variant->uref(set, cb, false);
> pr_debug("release set %s\n", set->name);
> - __ip_set_put_netlink(set);
> + __ip_set_put_swapping(set);
> }
> return 0;
> }
> @@ -1580,7 +1580,7 @@ ip_set_dump_do(struct sk_buff *skb, struct netlink_callback *cb)
> if (!cb->args[IPSET_CB_ARG0]) {
> /* Start listing: make sure set won't be destroyed */
> pr_debug("reference set\n");
> - set->ref_netlink++;
> + set->ref_swapping++;
> }
> write_unlock_bh(&ip_set_ref_lock);
> nlh = start_msg(skb, NETLINK_CB(cb->skb).portid,
> @@ -1646,11 +1646,11 @@ ip_set_dump_do(struct sk_buff *skb, struct netlink_callback *cb)
> release_refcount:
> /* If there was an error or set is done, release set */
> if (ret || !cb->args[IPSET_CB_ARG0]) {
> - set = ip_set_ref_netlink(inst, index);
> + set = ip_set_ref_swapping(inst, index);
> if (set->variant->uref)
> set->variant->uref(set, cb, false);
> pr_debug("release set %s\n", set->name);
> - __ip_set_put_netlink(set);
> + __ip_set_put_swapping(set);
> cb->args[IPSET_CB_ARG0] = 0;
> }
> out:
> @@ -1701,11 +1701,11 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb,
>
> do {
> if (retried) {
> - __ip_set_get_netlink(set);
> + __ip_set_get_swapping(set);
> nfnl_unlock(NFNL_SUBSYS_IPSET);
> cond_resched();
> nfnl_lock(NFNL_SUBSYS_IPSET);
> - __ip_set_put_netlink(set);
> + __ip_set_put_swapping(set);
> }
>
> ip_set_lock(set);
> --
> 2.17.1
>
>
--
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxx
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
