netdev - Re: [PATCH iproute2-next] iproute2: add 'ip monitor mcaddr' support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADXeF1GqzSWYmSFO3v6x7+KTc=Q+U9hUiTd+x5yvZaViSKSkOQ@mail.gmail.com>
Date: Mon, 18 Nov 2024 22:19:59 +0900
From: Yuyang Huang <yuyanghuang@...gle.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	David Ahern <dsahern@...nel.org>, roopa@...ulusnetworks.com, jiri@...nulli.us, 
	stephen@...workplumber.org, jimictw@...gle.com, prohr@...gle.com, 
	nicolas.dichtel@...nd.com, andrew@...n.ch, netdev@...r.kernel.org, 
	Maciej Żenczykowski <maze@...gle.com>, 
	Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [PATCH iproute2-next] iproute2: add 'ip monitor mcaddr' support

Thanks for the prompt review feedback.

>No need changes for headers. Stephen will sync the headers.

The patch will not compile without the header changes. I guess that
means I should put the patch on hold until the kernel change is merged
and the header changes get synced up to iproute2?

>The rtnl_add_nl_group() should be aligned with the upper bracket. e.g.

> Same with this one.

Acked, will fix the alignment in version 2.

Thanks,
Yuyang

Thanks,
Yuyang


On Mon, Nov 18, 2024 at 9:36 PM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> On Sun, Nov 17, 2024 at 11:16:55PM +0900, Yuyang Huang wrote:
> > Enhanced the 'ip monitor' command to track changes in IPv4 and IPv6
> > multicast addresses. This update allows the command to listen for
> > events related to multicast address additions and deletions by
> > registering to the newly introduced RTNLGRP_IPV4_MCADDR and
> > RTNLGRP_IPV6_MCADDR netlink groups.
> >
> > This patch depends on the kernel patch that adds RTNLGRP_IPV4_MCADDR
> > and RTNLGRP_IPV6_MCADDR being merged first.
> >
> > Here is an example usage:
> >
> > root@...-x86-64:/# ip monitor mcaddr
> > 8: nettest123    inet6 mcast ff01::1 scope global
> > 8: nettest123    inet6 mcast ff02::1 scope global
> > 8: nettest123    inet mcast 224.0.0.1 scope link
> > 8: nettest123    inet6 mcast ff02::1:ff00:7b01 scope global
> > Deleted 8: nettest123    inet mcast 224.0.0.1 scope link
> > Deleted 8: nettest123    inet6 mcast ff02::1:ff00:7b01 scope global
> > Deleted 8: nettest123    inet6 mcast ff02::1 scope global
> >
> > Cc: Maciej Żenczykowski <maze@...gle.com>
> > Cc: Lorenzo Colitti <lorenzo@...gle.com>
> > Signed-off-by: Yuyang Huang <yuyanghuang@...gle.com>
> > ---
> >  include/uapi/linux/rtnetlink.h |  8 ++++++++
> >  ip/ipaddress.c                 | 17 +++++++++++++++--
> >  ip/ipmonitor.c                 | 25 ++++++++++++++++++++++++-
> >  3 files changed, 47 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> > index 4e6c8e14..ccf26bf1 100644
> > --- a/include/uapi/linux/rtnetlink.h
> > +++ b/include/uapi/linux/rtnetlink.h
> > @@ -93,6 +93,10 @@ enum {
> >       RTM_NEWPREFIX   = 52,
> >  #define RTM_NEWPREFIX        RTM_NEWPREFIX
> >
> > +     RTM_NEWMULTICAST,
> > +#define RTM_NEWMULTICAST RTM_NEWMULTICAST
> > +     RTM_DELMULTICAST,
> > +#define RTM_DELMULTICAST RTM_DELMULTICAST
> >       RTM_GETMULTICAST = 58,
> >  #define RTM_GETMULTICAST RTM_GETMULTICAST
> >
> > @@ -772,6 +776,10 @@ enum rtnetlink_groups {
> >  #define RTNLGRP_TUNNEL               RTNLGRP_TUNNEL
> >       RTNLGRP_STATS,
> >  #define RTNLGRP_STATS                RTNLGRP_STATS
> > +     RTNLGRP_IPV4_MCADDR,
> > +#define RTNLGRP_IPV4_MCADDR  RTNLGRP_IPV4_MCADDR
> > +     RTNLGRP_IPV6_MCADDR,
> > +#define RTNLGRP_IPV6_MCADDR    RTNLGRP_IPV6_MCADDR
> >       __RTNLGRP_MAX
> >  };
> >  #define RTNLGRP_MAX  (__RTNLGRP_MAX - 1)
>
> No need changes for headers. Stephen will sync the headers.
>
> > @@ -220,6 +226,8 @@ int do_ipmonitor(int argc, char **argv)
> >                       lmask |= IPMON_LNEXTHOP;
> >               } else if (strcmp(*argv, "stats") == 0) {
> >                       lmask |= IPMON_LSTATS;
> > +             } else if (strcmp(*argv, "mcaddr") == 0) {
> > +                     lmask |= IPMON_LMCADDR;
> >               } else if (strcmp(*argv, "all") == 0) {
> >                       prefix_banner = 1;
> >               } else if (matches(*argv, "all-nsid") == 0) {
> > @@ -326,6 +334,21 @@ int do_ipmonitor(int argc, char **argv)
> >               exit(1);
> >       }
> >
> > +     if (lmask & IPMON_LMCADDR) {
> > +             if ((!preferred_family || preferred_family == AF_INET) &&
> > +                     rtnl_add_nl_group(&rth, RTNLGRP_IPV4_MCADDR) < 0) {
>
> The rtnl_add_nl_group() should be aligned with the upper bracket. e.g.
>
>                 if ((!preferred_family || preferred_family == AF_INET) &&
>                     rtnl_add_nl_group(&rth, RTNLGRP_IPV4_MCADDR) < 0) {
>
> > +                     fprintf(stderr,
> > +                             "Failed to add ipv4 mcaddr group to list\n");
> > +                     exit(1);
> > +             }
> > +             if ((!preferred_family || preferred_family == AF_INET6) &&
> > +                     rtnl_add_nl_group(&rth, RTNLGRP_IPV6_MCADDR) < 0) {
>
> Same with this one.
>
> Thanks
> Hangbin