Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 18 Jan 2024 05:04:30 -0800 | Subject | Re: [PATCH net] ipv6: mcast: fix data-race in ipv6_mc_down / mld_ifc_work | From | 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
| ![\](/images/icornerr.gif) |