Re: [PATCH net-next] ipv4: igmp: annotate data-races around im->users

[Uyo Ko]

>One suggestion: please add a Fixes: tag, like other data-race fixes
>annotated with READ_ONCE()/WRITE_ONCE().

Thanks for the prompt review, I will adjust it in patch v2.


On Thu, May 21, 2026 at 7:53 PM Jiayuan Chen <jiayuan.chen@xxxxxxxxx> wrote:
>
>
> On 5/21/26 5:36 PM, Yuyang Huang wrote:
> > /proc/net/igmp walks IPv4 multicast memberships under RCU and
> > prints im->users without holding RTNL, while multicast join and leave
> > paths update the field while holding RTNL. Annotate this intentional
> > lockless snapshot with READ_ONCE() and the matching writers with
> > WRITE_ONCE().
> >
> > Signed-off-by: Yuyang Huang <sigefriedhyy@xxxxxxxxx>
> > ---
> >   net/ipv4/igmp.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> > index 27d120183779..f2aca659b29c 100644
> > --- a/net/ipv4/igmp.c
> > +++ b/net/ipv4/igmp.c
> > @@ -1541,7 +1541,7 @@ static void ____ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
> >       }
> >
> >       if  (im) {
> > -             im->users++;
> > +             WRITE_ONCE(im->users, im->users + 1);
> >               ip_mc_add_src(in_dev, &addr, mode, 0, NULL, 0);
> >               goto out;
> >       }
> > @@ -1550,7 +1550,7 @@ static void ____ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
> >       if (!im)
> >               goto out;
> >
> > -     im->users = 1;
> > +     WRITE_ONCE(im->users, 1);
> >       im->interface = in_dev;
> >       in_dev_hold(in_dev);
> >       im->multiaddr = addr;
> > @@ -1784,7 +1784,10 @@ void __ip_mc_dec_group(struct in_device *in_dev, __be32 addr, gfp_t gfp)
> >            (i = rtnl_dereference(*ip)) != NULL;
> >            ip = &i->next_rcu) {
> >               if (i->multiaddr == addr) {
> > -                     if (--i->users == 0) {
> > +                     int new_users = i->users - 1;
> > +
> > +                     WRITE_ONCE(i->users, new_users);
> > +                     if (new_users == 0) {
>
> Using a local variable here feels slightly awkward, but it does save one
> extra load compared
>
> to re-reading i->users after the WRITE_ONCE(), so it's acceptable.
>
>
> One suggestion: please add a Fixes: tag, like other data-race fixes
> annotated with READ_ONCE()/WRITE_ONCE().