Received-SPF: pass (google.com: domain of linux-kernel+bounces-29927-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240117172102.12001-1-n.zhandarovich@fintech.ru>
In-Reply-To: <20240117172102.12001-1-n.zhandarovich@fintech.ru>
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 18 Jan 2024 09:59:54 +0100
Message-ID: <CANn89iLUxP_YGLD1mrCmAr9qSg7wPWDjWPhJHNa_X4QVyNWqBQ@mail.gmail.com>
Subject: Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
To: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Cc: "David S. Miller" <davem@davemloft.net>, David Ahern <dsahern@kernel.org>, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Taehee Yoo <ap420073@gmail.com>, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org, 
	syzbot+a9400cabb1d784e49abf@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 17, 2024 at 6:21=E2=80=AFPM Nikita Zhandarovich
<n.zhandarovich@fintech.ru> wrote:
>
> idev->mc_ifc_count can be written over without proper locking.
>
> Originally found by syzbot [1], fix this issue by encapsulating calls
> to mld_ifc_stop_work() (and mld_gq_stop_work() for good measure) with
> mutex_lock() and mutex_unlock() accordingly as these functions
> should only be called with mc_lock per their declarations.
>
> [1]
> BUG: KCSAN: data-race in ipv6_mc_down / mld_ifc_work
>
> Fixes: 2d9a93b4902b ("mld: convert from timer to delayed work")
> Reported-by: syzbot+a9400cabb1d784e49abf@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/000000000000994e09060ebcdffb@google.com=
/
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  net/ipv6/mcast.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index b75d3c9d41bb..bc6e0a0bad3c 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -2722,8 +2722,12 @@ void ipv6_mc_down(struct inet6_dev *idev)
>         synchronize_net();
>         mld_query_stop_work(idev);
>         mld_report_stop_work(idev);
> +
> +       mutex_lock(&idev->mc_lock);
>         mld_ifc_stop_work(idev);
>         mld_gq_stop_work(idev);
> +       mutex_unlock(&idev->mc_lock);
> +
>         mld_dad_stop_work(idev);
>  }
>

Thanks for the fix.

Reviewed-by: Eric Dumazet <edumazet@google.com>

I would also add some lockdep_assert_held() to make sure assumptions are me=
t.
Trading a comment for a runtime check is better.

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index b75d3c9d41bb5005af2d4e10fab58f157e9ea4fa..b256362d3b5d5111f649ebfee4f=
1557d8c063d92
100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1047,36 +1047,36 @@ bool ipv6_chk_mcast_addr(struct net_device
*dev, const struct in6_addr *group,
        return rv;
 }

-/* called with mc_lock */
 static void mld_gq_start_work(struct inet6_dev *idev)
 {
        unsigned long tv =3D get_random_u32_below(idev->mc_maxdelay);

+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_gq_running =3D 1;
        if (!mod_delayed_work(mld_wq, &idev->mc_gq_work, tv + 2))
                in6_dev_hold(idev);
 }

-/* called with mc_lock */
 static void mld_gq_stop_work(struct inet6_dev *idev)
 {
+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_gq_running =3D 0;
        if (cancel_delayed_work(&idev->mc_gq_work))
                __in6_dev_put(idev);
 }

-/* called with mc_lock */
 static void mld_ifc_start_work(struct inet6_dev *idev, unsigned long delay=
)
 {
        unsigned long tv =3D get_random_u32_below(delay);

+       lockdep_assert_held(&idev->mc_lock);
        if (!mod_delayed_work(mld_wq, &idev->mc_ifc_work, tv + 2))
                in6_dev_hold(idev);
 }

-/* called with mc_lock */
 static void mld_ifc_stop_work(struct inet6_dev *idev)
 {
+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_ifc_count =3D 0;
        if (cancel_delayed_work(&idev->mc_ifc_work))
                __in6_dev_put(idev);