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: [email protected]
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
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);
}
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: [email protected]
>> Link:
https://lore.kernel.org/all/[email protected]/
>> Signed-off-by: Nikita Zhandarovich <[email protected]>
>> ---
>> 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
On Wed, Jan 17, 2024 at 6:21 PM Nikita Zhandarovich
<[email protected]> 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: [email protected]
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> 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 <[email protected]>
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);
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
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: [email protected]
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> 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 <[email protected]>
On Thu, Jan 18, 2024 at 2:04 PM Nikita Zhandarovich
<[email protected]> 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.
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
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