[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CADXeF1EfgSKcz-zF24rsHUZiF+vUkiPsTmdFw=rf3EWCtcSk-A@mail.gmail.com>
Date: Wed, 4 Dec 2024 23:05:06 +0900
From: Yuyang Huang <yuyanghuang@...gle.com>
To: nicolas.dichtel@...nd.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,
liuhangbin@...il.com, andrew@...n.ch, netdev@...r.kernel.org,
Maciej Żenczykowski <maze@...gle.com>,
Lorenzo Colitti <lorenzo@...gle.com>, Patrick Ruddy <pruddy@...tta.att-mail.com>
Subject: Re: [PATCH net-next, v4] netlink: add IGMP/MLD join/leave notifications
>Maybe WARN_ON_ONCE() is enough?
Thank you very much for the prompt review feedback. Will update in patch v5.
Thanks,
Yuyang
On Wed, Dec 4, 2024 at 10:54 PM Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
>
> Le 04/12/2024 à 14:47, Yuyang Huang a écrit :
> > This change introduces netlink notifications for multicast address
> > changes. The following features are included:
> > * Addition and deletion of multicast addresses are reported using
> > RTM_NEWMULTICAST and RTM_DELMULTICAST messages with AF_INET and
> > AF_INET6.
> > * Two new notification groups: RTNLGRP_IPV4_MCADDR and
> > RTNLGRP_IPV6_MCADDR are introduced for receiving these events.
> >
> > This change allows user space applications (e.g., ip monitor) to
> > efficiently track multicast group memberships by listening for netlink
> > events. Previously, applications relied on inefficient polling of
> > procfs, introducing delays. With netlink notifications, applications
> > receive realtime updates on multicast group membership changes,
> > enabling more precise metrics collection and system monitoring.
> >
> > This change also unlocks the potential for implementing a wide range
> > of sophisticated multicast related features in user space by allowing
> > applications to combine kernel provided multicast address information
> > with user space data and communicate decisions back to the kernel for
> > more fine grained control. This mechanism can be used for various
> > purposes, including multicast filtering, IGMP/MLD offload, and
> > IGMP/MLD snooping.
> >
> > Cc: Maciej Żenczykowski <maze@...gle.com>
> > Cc: Lorenzo Colitti <lorenzo@...gle.com>
> > Co-developed-by: Patrick Ruddy <pruddy@...tta.att-mail.com>
> > Signed-off-by: Patrick Ruddy <pruddy@...tta.att-mail.com>
> > Link: https://lore.kernel.org/r/20180906091056.21109-1-pruddy@vyatta.att-mail.com
> > Signed-off-by: Yuyang Huang <yuyanghuang@...gle.com>
>
> A minor comment below and then:
> Acked-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
>
> > ---
> >
> > Changelog since v3:
> > - Remove unused variable 'scope' declaration.
> > - Align RTM_NEWMULTICAST and RTM_GETMULTICAST enum definitions with
> > existing code style.
> >
> > Changelog since v2:
> > - Use RT_SCOPE_UNIVERSE for both IGMP and MLD notification messages for
> > consistency.
> >
> > Changelog since v1:
> > - Implement MLD join/leave notifications.
> > - Revise the comment message to make it generic.
> > - Fix netdev/source_inline error.
> > - Reorder local variables according to "reverse xmas tree” style.
> >
> > include/uapi/linux/rtnetlink.h | 10 +++++-
> > net/ipv4/igmp.c | 53 +++++++++++++++++++++++++++++++
> > net/ipv6/mcast.c | 57 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> > index db7254d52d93..eccc0e7dcb7d 100644
> > --- a/include/uapi/linux/rtnetlink.h
> > +++ b/include/uapi/linux/rtnetlink.h
> > @@ -93,7 +93,11 @@ enum {
> > RTM_NEWPREFIX = 52,
> > #define RTM_NEWPREFIX RTM_NEWPREFIX
> >
> > - RTM_GETMULTICAST = 58,
> > + RTM_NEWMULTICAST = 56,
> > +#define RTM_NEWMULTICAST RTM_NEWMULTICAST
> > + RTM_DELMULTICAST,
> > +#define RTM_DELMULTICAST RTM_DELMULTICAST
> > + RTM_GETMULTICAST,
> > #define RTM_GETMULTICAST RTM_GETMULTICAST
> >
> > RTM_GETANYCAST = 62,
> > @@ -774,6 +778,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)
> > diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> > index 6a238398acc9..8d6ee19864c6 100644
> > --- a/net/ipv4/igmp.c
> > +++ b/net/ipv4/igmp.c
> > @@ -88,6 +88,7 @@
> > #include <linux/byteorder/generic.h>
> >
> > #include <net/net_namespace.h>
> > +#include <net/netlink.h>
> > #include <net/arp.h>
> > #include <net/ip.h>
> > #include <net/protocol.h>
> > @@ -1430,6 +1431,55 @@ static void ip_mc_hash_remove(struct in_device *in_dev,
> > *mc_hash = im->next_hash;
> > }
> >
> > +static int inet_fill_ifmcaddr(struct sk_buff *skb, struct net_device *dev,
> > + __be32 addr, int event)
> > +{
> > + struct ifaddrmsg *ifm;
> > + struct nlmsghdr *nlh;
> > +
> > + nlh = nlmsg_put(skb, 0, 0, event, sizeof(struct ifaddrmsg), 0);
> > + if (!nlh)
> > + return -EMSGSIZE;
> > +
> > + ifm = nlmsg_data(nlh);
> > + ifm->ifa_family = AF_INET;
> > + ifm->ifa_prefixlen = 32;
> > + ifm->ifa_flags = IFA_F_PERMANENT;
> > + ifm->ifa_scope = RT_SCOPE_UNIVERSE;
> > + ifm->ifa_index = dev->ifindex;
> > +
> > + if (nla_put_in_addr(skb, IFA_MULTICAST, addr) < 0) {
> > + nlmsg_cancel(skb, nlh);
> > + return -EMSGSIZE;
> > + }
> > +
> > + nlmsg_end(skb, nlh);
> > + return 0;
> > +}
> > +
> > +static void inet_ifmcaddr_notify(struct net_device *dev, __be32 addr, int event)
> > +{
> > + struct net *net = dev_net(dev);
> > + struct sk_buff *skb;
> > + int err = -ENOBUFS;
> > +
> > + skb = nlmsg_new(NLMSG_ALIGN(sizeof(struct ifaddrmsg))
> > + + nla_total_size(sizeof(__be32)), GFP_ATOMIC);
> > + if (!skb)
> > + goto error;
> > +
> > + err = inet_fill_ifmcaddr(skb, dev, addr, event);
> > + if (err < 0) {
> > + WARN_ON(err == -EMSGSIZE);
> Maybe WARN_ON_ONCE() is enough?
>
>
> Regards,
> Nicolas
Powered by blists - more mailing lists