[PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Nikita Zhandarovich]

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

write to 0xffff88813a80c832 of 1 bytes by task 3771 on cpu 0:
 mld_ifc_stop_work net/ipv6/mcast.c:1080 [inline]
 ipv6_mc_down+0x10a/0x280 net/ipv6/mcast.c:2725
 addrconf_ifdown+0xe32/0xf10 net/ipv6/addrconf.c:3949
 addrconf_notify+0x310/0x980
 notifier_call_chain kernel/notifier.c:93 [inline]
 raw_notifier_call_chain+0x6b/0x1c0 kernel/notifier.c:461
 __dev_notify_flags+0x205/0x3d0
 dev_change_flags+0xab/0xd0 net/core/dev.c:8685
 do_setlink+0x9f6/0x2430 net/core/rtnetlink.c:2916
 rtnl_group_changelink net/core/rtnetlink.c:3458 [inline]
 __rtnl_newlink net/core/rtnetlink.c:3717 [inline]
 rtnl_newlink+0xbb3/0x1670 net/core/rtnetlink.c:3754
 rtnetlink_rcv_msg+0x807/0x8c0 net/core/rtnetlink.c:6558
 netlink_rcv_skb+0x126/0x220 net/netlink/af_netlink.c:2545
 rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:6576
 netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
 netlink_unicast+0x589/0x650 net/netlink/af_netlink.c:1368
 netlink_sendmsg+0x66e/0x770 net/netlink/af_netlink.c:1910
 ...

write to 0xffff88813a80c832 of 1 bytes by task 22 on cpu 1:
 mld_ifc_work+0x54c/0x7b0 net/ipv6/mcast.c:2653
 process_one_work kernel/workqueue.c:2627 [inline]
 process_scheduled_works+0x5b8/0xa30 kernel/workqueue.c:2700
 worker_thread+0x525/0x730 kernel/workqueue.c:2781
 ...

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);
 }
 

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Hangbin Liu]

On Wed, Jan 17, 2024 at 09:21:02AM -0800, Nikita Zhandarovich 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
> 
> write to 0xffff88813a80c832 of 1 bytes by task 3771 on cpu 0:
>  mld_ifc_stop_work net/ipv6/mcast.c:1080 [inline]
>  ipv6_mc_down+0x10a/0x280 net/ipv6/mcast.c:2725
>  addrconf_ifdown+0xe32/0xf10 net/ipv6/addrconf.c:3949
>  addrconf_notify+0x310/0x980
>  notifier_call_chain kernel/notifier.c:93 [inline]
>  raw_notifier_call_chain+0x6b/0x1c0 kernel/notifier.c:461
>  __dev_notify_flags+0x205/0x3d0
>  dev_change_flags+0xab/0xd0 net/core/dev.c:8685
>  do_setlink+0x9f6/0x2430 net/core/rtnetlink.c:2916
>  rtnl_group_changelink net/core/rtnetlink.c:3458 [inline]
>  __rtnl_newlink net/core/rtnetlink.c:3717 [inline]
>  rtnl_newlink+0xbb3/0x1670 net/core/rtnetlink.c:3754
>  rtnetlink_rcv_msg+0x807/0x8c0 net/core/rtnetlink.c:6558
>  netlink_rcv_skb+0x126/0x220 net/netlink/af_netlink.c:2545
>  rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:6576
>  netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
>  netlink_unicast+0x589/0x650 net/netlink/af_netlink.c:1368
>  netlink_sendmsg+0x66e/0x770 net/netlink/af_netlink.c:1910
>  ...
> 
> write to 0xffff88813a80c832 of 1 bytes by task 22 on cpu 1:
>  mld_ifc_work+0x54c/0x7b0 net/ipv6/mcast.c:2653
>  process_one_work kernel/workqueue.c:2627 [inline]
>  process_scheduled_works+0x5b8/0xa30 kernel/workqueue.c:2700
>  worker_thread+0x525/0x730 kernel/workqueue.c:2781
>  ...
> 
> 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);
>  }
>  

I saw mld_process_v1() also cancel these works when changing to v1 mode.
Should we also add lock there?

Thanks
Hangbin

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Taehee Yoo]

On 1/18/24 11:45, Hangbin Liu wrote:

Hi Hangbin,

 > On Wed, Jan 17, 2024 at 09:21:02AM -0800, Nikita Zhandarovich 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
 >>
 >> write to 0xffff88813a80c832 of 1 bytes by task 3771 on cpu 0:
 >> mld_ifc_stop_work net/ipv6/mcast.c:1080 [inline]
 >> ipv6_mc_down+0x10a/0x280 net/ipv6/mcast.c:2725
 >> addrconf_ifdown+0xe32/0xf10 net/ipv6/addrconf.c:3949
 >> addrconf_notify+0x310/0x980
 >> notifier_call_chain kernel/notifier.c:93 [inline]
 >> raw_notifier_call_chain+0x6b/0x1c0 kernel/notifier.c:461
 >> __dev_notify_flags+0x205/0x3d0
 >> dev_change_flags+0xab/0xd0 net/core/dev.c:8685
 >> do_setlink+0x9f6/0x2430 net/core/rtnetlink.c:2916
 >> rtnl_group_changelink net/core/rtnetlink.c:3458 [inline]
 >> __rtnl_newlink net/core/rtnetlink.c:3717 [inline]
 >> rtnl_newlink+0xbb3/0x1670 net/core/rtnetlink.c:3754
 >> rtnetlink_rcv_msg+0x807/0x8c0 net/core/rtnetlink.c:6558
 >> netlink_rcv_skb+0x126/0x220 net/netlink/af_netlink.c:2545
 >> rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:6576
 >> netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
 >> netlink_unicast+0x589/0x650 net/netlink/af_netlink.c:1368
 >> netlink_sendmsg+0x66e/0x770 net/netlink/af_netlink.c:1910
 >> ...
 >>
 >> write to 0xffff88813a80c832 of 1 bytes by task 22 on cpu 1:
 >> mld_ifc_work+0x54c/0x7b0 net/ipv6/mcast.c:2653
 >> process_one_work kernel/workqueue.c:2627 [inline]
 >> process_scheduled_works+0x5b8/0xa30 kernel/workqueue.c:2700
 >> worker_thread+0x525/0x730 kernel/workqueue.c:2781
 >> ...
 >>
 >> 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);
 >> }
 >>
 >
 > I saw mld_process_v1() also cancel these works when changing to v1 mode.
 > Should we also add lock there?

I think mld_process_v1() doesn't have a problem.
Because mld_process_v1() is always called under mc_lock by mld_query_work().

mld_query_work()
    mutex_lock(&idev->mc_lock);
     __mld_query_work();
        mld_process_v1();
    mutex_unlock(&idev->mc_lock);

 >
 > Thanks
 > Hangbin

Thanks a lot,
Taehee Yoo

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Hangbin Liu]

On Thu, Jan 18, 2024 at 04:24:52PM +0900, Taehee Yoo wrote:
> > I saw mld_process_v1() also cancel these works when changing to v1 mode.
> > Should we also add lock there?
> 
> I think mld_process_v1() doesn't have a problem.
> Because mld_process_v1() is always called under mc_lock by mld_query_work().
> 
> mld_query_work()
>    mutex_lock(&idev->mc_lock);
>     __mld_query_work();
>        mld_process_v1();
>    mutex_unlock(&idev->mc_lock);

Thanks for this info, then this works for me.

Hangbin

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Taehee Yoo]

On 1/18/24 02:21, Nikita Zhandarovich wrote:

Hi Nikita,
Thanks a lot for this work!

 > 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
 >
 > write to 0xffff88813a80c832 of 1 bytes by task 3771 on cpu 0:
 > mld_ifc_stop_work net/ipv6/mcast.c:1080 [inline]
 > ipv6_mc_down+0x10a/0x280 net/ipv6/mcast.c:2725
 > addrconf_ifdown+0xe32/0xf10 net/ipv6/addrconf.c:3949
 > addrconf_notify+0x310/0x980
 > notifier_call_chain kernel/notifier.c:93 [inline]
 > raw_notifier_call_chain+0x6b/0x1c0 kernel/notifier.c:461
 > __dev_notify_flags+0x205/0x3d0
 > dev_change_flags+0xab/0xd0 net/core/dev.c:8685
 > do_setlink+0x9f6/0x2430 net/core/rtnetlink.c:2916
 > rtnl_group_changelink net/core/rtnetlink.c:3458 [inline]
 > __rtnl_newlink net/core/rtnetlink.c:3717 [inline]
 > rtnl_newlink+0xbb3/0x1670 net/core/rtnetlink.c:3754
 > rtnetlink_rcv_msg+0x807/0x8c0 net/core/rtnetlink.c:6558
 > netlink_rcv_skb+0x126/0x220 net/netlink/af_netlink.c:2545
 > rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:6576
 > netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
 > netlink_unicast+0x589/0x650 net/netlink/af_netlink.c:1368
 > netlink_sendmsg+0x66e/0x770 net/netlink/af_netlink.c:1910
 > ...
 >
 > write to 0xffff88813a80c832 of 1 bytes by task 22 on cpu 1:
 > mld_ifc_work+0x54c/0x7b0 net/ipv6/mcast.c:2653
 > process_one_work kernel/workqueue.c:2627 [inline]
 > process_scheduled_works+0x5b8/0xa30 kernel/workqueue.c:2700
 > worker_thread+0x525/0x730 kernel/workqueue.c:2781
 > ...
 >
 > 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);
 > }
 >

Acked-by: Taehee Yoo <ap420073@gmail.com>

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Eric Dumazet]

On Wed, Jan 17, 2024 at 6:21 PM 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 met.
Trading a comment for a runtime check is better.

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index b75d3c9d41bb5005af2d4e10fab58f157e9ea4fa..b256362d3b5d5111f649ebfee4f1557d8c063d92
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 = get_random_u32_below(idev->mc_maxdelay);

+       lockdep_assert_held(&idev->mc_lock);
        idev->mc_gq_running = 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 = 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 = 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 = 0;
        if (cancel_delayed_work(&idev->mc_ifc_work))
                __in6_dev_put(idev);

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Nikita Zhandarovich]

Hello,

On 1/18/24 00:59, Eric Dumazet wrote:
> On Wed, Jan 17, 2024 at 6:21 PM 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 met.
> Trading a comment for a runtime check is better.
> 
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index b75d3c9d41bb5005af2d4e10fab58f157e9ea4fa..b256362d3b5d5111f649ebfee4f1557d8c063d92
> 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 = get_random_u32_below(idev->mc_maxdelay);
> 
> +       lockdep_assert_held(&idev->mc_lock);
>         idev->mc_gq_running = 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 = 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 = 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 = 0;
>         if (cancel_delayed_work(&idev->mc_ifc_work))
>                 __in6_dev_put(idev);

Just to clarify: should I incorporate your change into v2 version of my
original one and attach 'Reviewed-by' tags or should I send a different
patch with your suggestion?

Apologies for the possibly silly question, got a little confused by
signals from multiple maintainers.

With regards,
Nikita

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Eric Dumazet]

On Thu, Jan 18, 2024 at 2:04 PM Nikita Zhandarovich
<n.zhandarovich@fintech.ru> wrote:

> Just to clarify: should I incorporate your change into v2 version of my
> original one and attach 'Reviewed-by' tags or should I send a different
> patch with your suggestion?
>
> Apologies for the possibly silly question, got a little confused by
> signals from multiple maintainers.
>

No worries, we can wait for net-next being open (next week) for adding
these lockdep annotations.

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[Hangbin Liu]

On Wed, Jan 17, 2024 at 09:21:02AM -0800, Nikita Zhandarovich 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
> 
> write to 0xffff88813a80c832 of 1 bytes by task 3771 on cpu 0:
>  mld_ifc_stop_work net/ipv6/mcast.c:1080 [inline]
>  ipv6_mc_down+0x10a/0x280 net/ipv6/mcast.c:2725
>  addrconf_ifdown+0xe32/0xf10 net/ipv6/addrconf.c:3949
>  addrconf_notify+0x310/0x980
>  notifier_call_chain kernel/notifier.c:93 [inline]
>  raw_notifier_call_chain+0x6b/0x1c0 kernel/notifier.c:461
>  __dev_notify_flags+0x205/0x3d0
>  dev_change_flags+0xab/0xd0 net/core/dev.c:8685
>  do_setlink+0x9f6/0x2430 net/core/rtnetlink.c:2916
>  rtnl_group_changelink net/core/rtnetlink.c:3458 [inline]
>  __rtnl_newlink net/core/rtnetlink.c:3717 [inline]
>  rtnl_newlink+0xbb3/0x1670 net/core/rtnetlink.c:3754
>  rtnetlink_rcv_msg+0x807/0x8c0 net/core/rtnetlink.c:6558
>  netlink_rcv_skb+0x126/0x220 net/netlink/af_netlink.c:2545
>  rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:6576
>  netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
>  netlink_unicast+0x589/0x650 net/netlink/af_netlink.c:1368
>  netlink_sendmsg+0x66e/0x770 net/netlink/af_netlink.c:1910
>  ...
> 
> write to 0xffff88813a80c832 of 1 bytes by task 22 on cpu 1:
>  mld_ifc_work+0x54c/0x7b0 net/ipv6/mcast.c:2653
>  process_one_work kernel/workqueue.c:2627 [inline]
>  process_scheduled_works+0x5b8/0xa30 kernel/workqueue.c:2700
>  worker_thread+0x525/0x730 kernel/workqueue.c:2781
>  ...
> 
> 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);
>  }
>  

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 17 Jan 2024 09:21:02 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - [net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
    https://git.kernel.org/netdev/net/c/2e7ef287f07c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html