* [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
@ 2024-01-17 17:21 Nikita Zhandarovich
2024-01-18 2:45 ` Hangbin Liu
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Nikita Zhandarovich @ 2024-01-17 17:21 UTC (permalink / raw
To: David S. Miller
Cc: Nikita Zhandarovich, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Taehee Yoo, netdev, linux-kernel,
syzbot+a9400cabb1d784e49abf
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);
}
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-17 17:21 [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work Nikita Zhandarovich
@ 2024-01-18 2:45 ` Hangbin Liu
2024-01-18 7:24 ` Taehee Yoo
2024-01-18 7:11 ` Taehee Yoo
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2024-01-18 2:45 UTC (permalink / raw
To: Nikita Zhandarovich
Cc: David S. Miller, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Taehee Yoo, netdev, linux-kernel,
syzbot+a9400cabb1d784e49abf
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-17 17:21 [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work Nikita Zhandarovich
2024-01-18 2:45 ` Hangbin Liu
@ 2024-01-18 7:11 ` Taehee Yoo
2024-01-18 8:59 ` Eric Dumazet
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Taehee Yoo @ 2024-01-18 7:11 UTC (permalink / raw
To: Nikita Zhandarovich, David S. Miller
Cc: David Ahern, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev,
linux-kernel, syzbot+a9400cabb1d784e49abf
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-18 2:45 ` Hangbin Liu
@ 2024-01-18 7:24 ` Taehee Yoo
2024-01-18 9:55 ` Hangbin Liu
0 siblings, 1 reply; 10+ messages in thread
From: Taehee Yoo @ 2024-01-18 7:24 UTC (permalink / raw
To: Hangbin Liu, Nikita Zhandarovich
Cc: David S. Miller, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel, syzbot+a9400cabb1d784e49abf
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-17 17:21 [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work Nikita Zhandarovich
2024-01-18 2:45 ` Hangbin Liu
2024-01-18 7:11 ` Taehee Yoo
@ 2024-01-18 8:59 ` Eric Dumazet
2024-01-18 13:04 ` Nikita Zhandarovich
2024-01-18 9:56 ` Hangbin Liu
2024-01-18 18:00 ` patchwork-bot+netdevbpf
4 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2024-01-18 8:59 UTC (permalink / raw
To: Nikita Zhandarovich
Cc: David S. Miller, David Ahern, Jakub Kicinski, Paolo Abeni,
Taehee Yoo, netdev, linux-kernel, syzbot+a9400cabb1d784e49abf
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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-18 7:24 ` Taehee Yoo
@ 2024-01-18 9:55 ` Hangbin Liu
0 siblings, 0 replies; 10+ messages in thread
From: Hangbin Liu @ 2024-01-18 9:55 UTC (permalink / raw
To: Taehee Yoo
Cc: Nikita Zhandarovich, David S. Miller, David Ahern, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
syzbot+a9400cabb1d784e49abf
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-17 17:21 [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work Nikita Zhandarovich
` (2 preceding siblings ...)
2024-01-18 8:59 ` Eric Dumazet
@ 2024-01-18 9:56 ` Hangbin Liu
2024-01-18 18:00 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 10+ messages in thread
From: Hangbin Liu @ 2024-01-18 9:56 UTC (permalink / raw
To: Nikita Zhandarovich
Cc: David S. Miller, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Taehee Yoo, netdev, linux-kernel,
syzbot+a9400cabb1d784e49abf
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-18 8:59 ` Eric Dumazet
@ 2024-01-18 13:04 ` Nikita Zhandarovich
2024-01-18 13:28 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Nikita Zhandarovich @ 2024-01-18 13:04 UTC (permalink / raw
To: Eric Dumazet
Cc: David S. Miller, David Ahern, Jakub Kicinski, Paolo Abeni,
Taehee Yoo, netdev, linux-kernel, syzbot+a9400cabb1d784e49abf
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-18 13:04 ` Nikita Zhandarovich
@ 2024-01-18 13:28 ` Eric Dumazet
0 siblings, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2024-01-18 13:28 UTC (permalink / raw
To: Nikita Zhandarovich
Cc: David S. Miller, David Ahern, Jakub Kicinski, Paolo Abeni,
Taehee Yoo, netdev, linux-kernel, syzbot+a9400cabb1d784e49abf
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work
2024-01-17 17:21 [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work Nikita Zhandarovich
` (3 preceding siblings ...)
2024-01-18 9:56 ` Hangbin Liu
@ 2024-01-18 18:00 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-01-18 18:00 UTC (permalink / raw
To: Nikita Zhandarovich
Cc: davem, dsahern, edumazet, kuba, pabeni, ap420073, netdev,
linux-kernel, syzbot+a9400cabb1d784e49abf
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-01-18 18:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-17 17:21 [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work Nikita Zhandarovich
2024-01-18 2:45 ` Hangbin Liu
2024-01-18 7:24 ` Taehee Yoo
2024-01-18 9:55 ` Hangbin Liu
2024-01-18 7:11 ` Taehee Yoo
2024-01-18 8:59 ` Eric Dumazet
2024-01-18 13:04 ` Nikita Zhandarovich
2024-01-18 13:28 ` Eric Dumazet
2024-01-18 9:56 ` Hangbin Liu
2024-01-18 18:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).