net/ipv6/mcast.c | 4 ++++ 1 file changed, 4 insertions(+)
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);
}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>
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);
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
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.
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>
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
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
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
© 2016 - 2025 Red Hat, Inc.