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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Link: https://lore.kernel.org/all/000000000000994e09060ebcdffb@xxxxxxxxxx/
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxx>
> 
> 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