Hi Jozsef,
On Mon, Sep 4, 2023 at 11:45 PM Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxx> wrote:
>
> Hi Kyle,
>
> On Mon, 4 Sep 2023, Kyle Zeng wrote:
>
> > There is a race between IPSET_CMD_ADD and IPSET_CMD_SWAP in
> > netfilter/ip_set, which can lead to the invocation of `__ip_set_put` on
> > a wrong `set`, triggering the `BUG_ON(set->ref == 0);` check in it.
> > ......
> > A proof-of-concept code that can trigger the bug is attached.
> >
> > The bug is confirmed on v5.10, v6.1, v6.5.rc7 and upstream.
>
> Thanks for the thorough report. I think the proper fix is to change the
> reference counter at rescheduling from "ref" to "ref_netlink", which
> protects long taking procedures (originally just dumping). Could you
> verify that the patch below fixes the issue?
>
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index e564b5174261..8a9cea8ed5ed 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -682,6 +682,15 @@ __ip_set_put(struct ip_set *set)
> /* set->ref can be swapped out by ip_set_swap, netlink events (like dump) need
> * a separate reference counter
> */
> +static void
> +__ip_set_get_netlink(struct ip_set *set)
> +{
> + write_lock_bh(&ip_set_ref_lock);
> + set->ref_netlink++;
> + write_unlock_bh(&ip_set_ref_lock);
> +}
> +
> +
> static void
> __ip_set_put_netlink(struct ip_set *set)
> {
> @@ -1693,11 +1702,11 @@ call_ad(struct net *net, struct sock *ctnl, struct sk_buff *skb,
>
> do {
> if (retried) {
> - __ip_set_get(set);
> + __ip_set_get_netlink(set);
> nfnl_unlock(NFNL_SUBSYS_IPSET);
> cond_resched();
> nfnl_lock(NFNL_SUBSYS_IPSET);
> - __ip_set_put(set);
> + __ip_set_put_netlink(set);
> }
>
> ip_set_lock(set);
>
Sorry for the late reply, somehow the response was moved to spam folder and I
didn't notice it.
> Thanks for the thorough report. I think the proper fix is to change the
> reference counter at rescheduling from "ref" to "ref_netlink", which
> protects long taking procedures (originally just dumping). Could you
> verify that the patch below fixes the issue?
I applied the patch to the upstream kernel and tested it. The proof-of-concept
crash program can no longer trigger the crash.
Best,
Kyle Zeng
