[PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Yuyang Huang]

Extended RTM_GETMULTICAST to support dumping joined IPv4 multicast
addresses, in addition to the existing IPv6 functionality. This allows
userspace applications to retrieve both IPv4 and IPv6 multicast
addresses through similar netlink command and then monitor future
changes by registering to RTNLGRP_IPV4_MCADDR and RTNLGRP_IPV6_MCADDR.

Cc: Maciej Żenczykowski <maze@google.com>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Yuyang Huang <yuyanghuang@google.com>
---

Changelog since v3:
- Refactor in_dev_dump_addr() to break down the logic into two separate functions to simplify the
  logic.

Changelog since v2:
- Fix checkpatch.pl warnings.
- Remove one redundant EXPORT_SYMBOL().

Changelog since v1:
- Minor style fixes.
- Use for_each_pmc_rcu() instead of for_each_pmc_rtnl().

 include/linux/igmp.h |  2 ++
 net/ipv4/devinet.c   | 73 +++++++++++++++++++++++++++++++++++++++++---
 net/ipv4/igmp.c      |  8 ++---
 3 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/include/linux/igmp.h b/include/linux/igmp.h
index 073b30a9b850..757c0aeea1ac 100644
--- a/include/linux/igmp.h
+++ b/include/linux/igmp.h
@@ -142,4 +142,6 @@ extern void __ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
 extern void ip_mc_inc_group(struct in_device *in_dev, __be32 addr);
 int ip_mc_check_igmp(struct sk_buff *skb);
 
+int inet_fill_ifmcaddr(struct sk_buff *skb, struct net_device *dev,
+		       const struct ip_mc_list *im, int event, int flags);
 #endif
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index c8b3cf5fba4c..19b3cea8da2b 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -114,6 +114,7 @@ struct inet_fill_args {
 	unsigned int flags;
 	int netnsid;
 	int ifindex;
+	enum addr_type_t type;
 };
 
 #define IN4_ADDR_HSIZE_SHIFT	8
@@ -1846,9 +1847,39 @@ static int inet_valid_dump_ifaddr_req(const struct nlmsghdr *nlh,
 	return 0;
 }
 
-static int in_dev_dump_addr(struct in_device *in_dev, struct sk_buff *skb,
-			    struct netlink_callback *cb, int *s_ip_idx,
-			    struct inet_fill_args *fillargs)
+static int in_dev_dump_ifmcaddr(struct in_device *in_dev, struct sk_buff *skb,
+				struct netlink_callback *cb, int *s_ip_idx,
+				struct inet_fill_args *fillargs)
+{
+	struct ip_mc_list *im;
+	int ip_idx = 0;
+	int err;
+
+	for (im = rcu_dereference(in_dev->mc_list);
+	     im;
+	     im = rcu_dereference(im->next_rcu)) {
+		if (ip_idx < *s_ip_idx) {
+			ip_idx++;
+			continue;
+		}
+		err = inet_fill_ifmcaddr(skb, in_dev->dev, im,
+					 RTM_GETMULTICAST, NLM_F_MULTI);
+		if (err < 0)
+			goto done;
+
+		nl_dump_check_consistent(cb, nlmsg_hdr(skb));
+		ip_idx++;
+	}
+	err = 0;
+	ip_idx = 0;
+done:
+	*s_ip_idx = ip_idx;
+	return err;
+}
+
+static int in_dev_dump_ifaddr(struct in_device *in_dev, struct sk_buff *skb,
+			      struct netlink_callback *cb, int *s_ip_idx,
+			      struct inet_fill_args *fillargs)
 {
 	struct in_ifaddr *ifa;
 	int ip_idx = 0;
@@ -1874,6 +1905,21 @@ static int in_dev_dump_addr(struct in_device *in_dev, struct sk_buff *skb,
 	return err;
 }
 
+static int in_dev_dump_addr(struct in_device *in_dev, struct sk_buff *skb,
+			    struct netlink_callback *cb, int *s_ip_idx,
+			    struct inet_fill_args *fillargs)
+{
+	switch (fillargs->type) {
+	case UNICAST_ADDR:
+		return in_dev_dump_ifaddr(in_dev, skb, cb, s_ip_idx, fillargs);
+	case MULTICAST_ADDR:
+		return in_dev_dump_ifmcaddr(in_dev, skb, cb, s_ip_idx,
+					    fillargs);
+	default:
+		return 0;
+	}
+}
+
 /* Combine dev_addr_genid and dev_base_seq to detect changes.
  */
 static u32 inet_base_seq(const struct net *net)
@@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
 	return res;
 }
 
-static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
+static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
+			  enum addr_type_t type)
 {
 	const struct nlmsghdr *nlh = cb->nlh;
 	struct inet_fill_args fillargs = {
 		.portid = NETLINK_CB(cb->skb).portid,
 		.seq = nlh->nlmsg_seq,
-		.event = RTM_NEWADDR,
 		.flags = NLM_F_MULTI,
 		.netnsid = -1,
+		.type = type,
 	};
 	struct net *net = sock_net(skb->sk);
 	struct net *tgt_net = net;
@@ -1949,6 +1996,20 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
 	return err;
 }
 
+static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	enum addr_type_t type = UNICAST_ADDR;
+
+	return inet_dump_addr(skb, cb, type);
+}
+
+static int inet_dump_ifmcaddr(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	enum addr_type_t type = MULTICAST_ADDR;
+
+	return inet_dump_addr(skb, cb, type);
+}
+
 static void rtmsg_ifa(int event, struct in_ifaddr *ifa, struct nlmsghdr *nlh,
 		      u32 portid)
 {
@@ -2845,6 +2906,8 @@ static const struct rtnl_msg_handler devinet_rtnl_msg_handlers[] __initconst = {
 	{.protocol = PF_INET, .msgtype = RTM_GETNETCONF,
 	 .doit = inet_netconf_get_devconf, .dumpit = inet_netconf_dump_devconf,
 	 .flags = RTNL_FLAG_DOIT_UNLOCKED | RTNL_FLAG_DUMP_UNLOCKED},
+	{.owner = THIS_MODULE, .protocol = PF_INET, .msgtype = RTM_GETMULTICAST,
+	 .dumpit = inet_dump_ifmcaddr, .flags = RTNL_FLAG_DUMP_UNLOCKED},
 };
 
 void __init devinet_init(void)
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 3da126cea884..3e6faaab54c0 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1432,14 +1432,14 @@ 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,
-			      const struct ip_mc_list *im, int event)
+int inet_fill_ifmcaddr(struct sk_buff *skb, struct net_device *dev,
+		       const struct ip_mc_list *im, int event, int flags)
 {
 	struct ifa_cacheinfo ci;
 	struct ifaddrmsg *ifm;
 	struct nlmsghdr *nlh;
 
-	nlh = nlmsg_put(skb, 0, 0, event, sizeof(struct ifaddrmsg), 0);
+	nlh = nlmsg_put(skb, 0, 0, event, sizeof(struct ifaddrmsg), flags);
 	if (!nlh)
 		return -EMSGSIZE;
 
@@ -1479,7 +1479,7 @@ static void inet_ifmcaddr_notify(struct net_device *dev,
 	if (!skb)
 		goto error;
 
-	err = inet_fill_ifmcaddr(skb, dev, im, event);
+	err = inet_fill_ifmcaddr(skb, dev, im, event, 0);
 	if (err < 0) {
 		WARN_ON_ONCE(err == -EMSGSIZE);
 		nlmsg_free(skb);
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Paolo Abeni]

On 1/9/25 8:22 AM, Yuyang Huang wrote:
> @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
>  	return res;
>  }
>  
> -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> +			  enum addr_type_t type)
>  {
>  	const struct nlmsghdr *nlh = cb->nlh;
>  	struct inet_fill_args fillargs = {
>  		.portid = NETLINK_CB(cb->skb).portid,
>  		.seq = nlh->nlmsg_seq,
> -		.event = RTM_NEWADDR,
>  		.flags = NLM_F_MULTI,
>  		.netnsid = -1,
> +		.type = type,

This patch is apparently breaking a few tests:

https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-net-dbg&pw-n=0&pass=0
https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-nf-dbg&pw-n=0&pass=0
https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-nf&pw-n=0&pass=0

I suspect the above chunk confuses the user-space as inet_fill_ifaddr()
still get the 'event' value from fillargs->event

Also, this will need a paired self-test - even something very simple
just exercising the new code.

Thanks,

Paolo

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Yuyang Huang]

Hi Paolo

Thanks for the review feedback.

>I suspect the above chunk confuses the user-space as inet_fill_ifaddr()
>still get the 'event' value from fillargs->event

You're right, I missed initializing the event field in the fillargs
structure. It's a bug introduced in the v4 patch during refactoring. I
will fix it in the v5 patch.

>Also, this will need a paired self-test - even something very simple
>just exercising the new code.

Sure, I can send separate patch for adding self-test, but please let
me know if I should include the self-test in the same patch

Thanks
Yuyang


On Thu, Jan 9, 2025 at 8:30 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 1/9/25 8:22 AM, Yuyang Huang wrote:
> > @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
> >       return res;
> >  }
> >
> > -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> > +                       enum addr_type_t type)
> >  {
> >       const struct nlmsghdr *nlh = cb->nlh;
> >       struct inet_fill_args fillargs = {
> >               .portid = NETLINK_CB(cb->skb).portid,
> >               .seq = nlh->nlmsg_seq,
> > -             .event = RTM_NEWADDR,
> >               .flags = NLM_F_MULTI,
> >               .netnsid = -1,
> > +             .type = type,
>
> This patch is apparently breaking a few tests:
>
> https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-net-dbg&pw-n=0&pass=0
> https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-nf-dbg&pw-n=0&pass=0
> https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-nf&pw-n=0&pass=0
>
> I suspect the above chunk confuses the user-space as inet_fill_ifaddr()
> still get the 'event' value from fillargs->event
>
> Also, this will need a paired self-test - even something very simple
> just exercising the new code.
>
> Thanks,
>
> Paolo
>

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Yuyang Huang]

>Also, this will need a paired self-test - even something very simple
>just exercising the new code.

After looking into the existing selftest, I feel writing C program
selftests might be a little bit anti-pattern. The existing test cases
primarily use iproute2. This approach appears to be more common and
easier to implement. I do plan to migrate `ip maddress` to use netlink
instead of parsing procfs, which enables me to write selftests using
iproute2.

I intend to add similar selftests for my other patches
(anycast/multicast notifications), which already have corresponding
iproute2 patches under review or merged.

For this patch, my proposed steps for adding test are:

1. Fix this patch and get it merged.
2. Update iproute2(`ip maddress`) to use netlink.
3. Write selftests using iproute2.

Please let me know if you have any concerns.

Thanks,
Yuyang

On Thu, Jan 9, 2025 at 9:04 PM Yuyang Huang <yuyanghuang@google.com> wrote:
>
> Hi Paolo
>
> Thanks for the review feedback.
>
> >I suspect the above chunk confuses the user-space as inet_fill_ifaddr()
> >still get the 'event' value from fillargs->event
>
> You're right, I missed initializing the event field in the fillargs
> structure. It's a bug introduced in the v4 patch during refactoring. I
> will fix it in the v5 patch.
>
> >Also, this will need a paired self-test - even something very simple
> >just exercising the new code.
>
> Sure, I can send separate patch for adding self-test, but please let
> me know if I should include the self-test in the same patch
>
> Thanks
> Yuyang
>
>
> On Thu, Jan 9, 2025 at 8:30 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On 1/9/25 8:22 AM, Yuyang Huang wrote:
> > > @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
> > >       return res;
> > >  }
> > >
> > > -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> > > +                       enum addr_type_t type)
> > >  {
> > >       const struct nlmsghdr *nlh = cb->nlh;
> > >       struct inet_fill_args fillargs = {
> > >               .portid = NETLINK_CB(cb->skb).portid,
> > >               .seq = nlh->nlmsg_seq,
> > > -             .event = RTM_NEWADDR,
> > >               .flags = NLM_F_MULTI,
> > >               .netnsid = -1,
> > > +             .type = type,
> >
> > This patch is apparently breaking a few tests:
> >
> > https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-net-dbg&pw-n=0&pass=0
> > https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-nf-dbg&pw-n=0&pass=0
> > https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-01-09--09-00&executor=vmksft-nf&pw-n=0&pass=0
> >
> > I suspect the above chunk confuses the user-space as inet_fill_ifaddr()
> > still get the 'event' value from fillargs->event
> >
> > Also, this will need a paired self-test - even something very simple
> > just exercising the new code.
> >
> > Thanks,
> >
> > Paolo
> >

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Paolo Abeni]

On 1/9/25 4:01 PM, Yuyang Huang wrote:
>> Also, this will need a paired self-test - even something very simple
>> just exercising the new code.
> 
> After looking into the existing selftest, I feel writing C program
> selftests might be a little bit anti-pattern. The existing test cases
> primarily use iproute2. This approach appears to be more common and
> easier to implement. I do plan to migrate `ip maddress` to use netlink
> instead of parsing procfs, which enables me to write selftests using
> iproute2.
> 
> I intend to add similar selftests for my other patches
> (anycast/multicast notifications), which already have corresponding
> iproute2 patches under review or merged.
> 
> For this patch, my proposed steps for adding test are:
> 
> 1. Fix this patch and get it merged.
> 2. Update iproute2(`ip maddress`) to use netlink.
> 3. Write selftests using iproute2.
> 
> Please let me know if you have any concerns.

Fine by me. FTR we used the C program way for mptcp - before ynl advent
- and it was quite a pain.

Cheers,

Paolo

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[David Ahern]

On 1/9/25 12:22 AM, Yuyang Huang wrote:
> @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
>  	return res;
>  }
>  
> -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> +			  enum addr_type_t type)
>  {
>  	const struct nlmsghdr *nlh = cb->nlh;
>  	struct inet_fill_args fillargs = {
>  		.portid = NETLINK_CB(cb->skb).portid,
>  		.seq = nlh->nlmsg_seq,
> -		.event = RTM_NEWADDR,
>  		.flags = NLM_F_MULTI,
>  		.netnsid = -1,
> +		.type = type,

my comment meant that this `type` should be removed and the wrappers
below just call the intended function. No need for the extra layers.

>  	};
>  	struct net *net = sock_net(skb->sk);
>  	struct net *tgt_net = net;
> @@ -1949,6 +1996,20 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>  	return err;
>  }
>  
> +static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	enum addr_type_t type = UNICAST_ADDR;
> +
> +	return inet_dump_addr(skb, cb, type);
> +}
> +
> +static int inet_dump_ifmcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	enum addr_type_t type = MULTICAST_ADDR;
> +
> +	return inet_dump_addr(skb, cb, type);
> +}
> +
>  static void rtmsg_ifa(int event, struct in_ifaddr *ifa, struct nlmsghdr *nlh,
>  		      u32 portid)
>  {

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Yuyang Huang]

>my comment meant that this `type` should be removed and the wrappers
>below just call the intended function. No need for the extra layers.

Sorry, I still do not fully understand the suggestions.

In the current inet_dump_ifaddr() function, there are two places where
in_dev_dump_addr() is called.

For example, we have the following code snippet.

>if (!in_dev)
>goto done;
>err = in_dev_dump_addr(in_dev, skb, cb, &ctx->ip_idx,
>       &fillargs);
>goto done;
>}

Do you suggest we do the following way?

> If (type == UNICAST_ADDR)
>    err = in_dev_dump_ifaddr(in_dev, skb, cb, &ctx->ip_idx,
>                                             &fillargs);
> else if (type == MULTICAST_ADDR)
>    in_dev_dump_ifmcaddr(in_dev, skb, cb, s_ip_idx,
>                                         &fillargs);

The current functional call stack is as follows:

inet_dump_ifaddr()/inet_dump_ifmcaddr() -> inet_dump_addr() ->
in_dev_dump_ifaddr()/in_dev_dump_ifmcaddr().

The ifaddr and ifmcaddr dump code paths share common logic inside
inet_dump_addr(). If we don't do the dispatching in
in_dev_dump_addr(), we have to do the dispatching in inet_dump_addr()
instead, and the dispatching logic will be duplicated twice. I don't
think this will simplify the code.

Or do you suggest I should pass a function pointer for
in_dev_dump_ifaddr()/in_dev_dump_ifmcaddr() into inet_dump_addr()?

Thanks,

Yuyang

On Fri, Jan 10, 2025 at 12:33 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 1/9/25 12:22 AM, Yuyang Huang wrote:
> > @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
> >       return res;
> >  }
> >
> > -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> > +                       enum addr_type_t type)
> >  {
> >       const struct nlmsghdr *nlh = cb->nlh;
> >       struct inet_fill_args fillargs = {
> >               .portid = NETLINK_CB(cb->skb).portid,
> >               .seq = nlh->nlmsg_seq,
> > -             .event = RTM_NEWADDR,
> >               .flags = NLM_F_MULTI,
> >               .netnsid = -1,
> > +             .type = type,
>
> my comment meant that this `type` should be removed and the wrappers
> below just call the intended function. No need for the extra layers.
>
> >       };
> >       struct net *net = sock_net(skb->sk);
> >       struct net *tgt_net = net;
> > @@ -1949,6 +1996,20 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> >       return err;
> >  }
> >
> > +static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > +     enum addr_type_t type = UNICAST_ADDR;
> > +
> > +     return inet_dump_addr(skb, cb, type);
> > +}
> > +
> > +static int inet_dump_ifmcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > +     enum addr_type_t type = MULTICAST_ADDR;
> > +
> > +     return inet_dump_addr(skb, cb, type);
> > +}
> > +
> >  static void rtmsg_ifa(int event, struct in_ifaddr *ifa, struct nlmsghdr *nlh,
> >                     u32 portid)
> >  {
>

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Jakub Kicinski]

On Fri, 10 Jan 2025 00:01:31 +0900 Yuyang Huang wrote:
> For this patch, my proposed steps for adding test are:
> 
> 1. Fix this patch and get it merged.
> 2. Update iproute2(`ip maddress`) to use netlink.
> 3. Write selftests using iproute2.

You don't have to wait for iproute2 changes to be merged.
Push them somewhere we can pull from, and when you post v5 
(with the self-test) just add a link to that iproute2 git repo
to the cover letter. 
We'll pull your changes to the iproute2 used by the CI image.

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Yuyang Huang]

>my comment meant that this `type` should be removed and the wrappers
>below just call the intended function. No need for the extra layers.

This patch was trying to follow the similar pattern in addrconf.c. We
have a similar addr_type_t based dispatching mechanism to handle the
dumping of ANYCAST, MULTICAST, and UNICAST addresses.

inet6_dump_ifaddr()/inet6_dump_ifmcaddr()/inet6_dump_ifacaddr() ->
inet6_dump_addr() -> in6_dump_addrs()

The dispatching switch statement resides within in6_dump_addrs(), and
those dump functions share the common code path in inet6_dump_addr().

Thanks,
Yuyang


On Fri, Jan 10, 2025 at 12:52 AM Yuyang Huang <yuyanghuang@google.com> wrote:
>
> >my comment meant that this `type` should be removed and the wrappers
> >below just call the intended function. No need for the extra layers.
>
> Sorry, I still do not fully understand the suggestions.
>
> In the current inet_dump_ifaddr() function, there are two places where
> in_dev_dump_addr() is called.
>
> For example, we have the following code snippet.
>
> >if (!in_dev)
> >goto done;
> >err = in_dev_dump_addr(in_dev, skb, cb, &ctx->ip_idx,
> >       &fillargs);
> >goto done;
> >}
>
> Do you suggest we do the following way?
>
> > If (type == UNICAST_ADDR)
> >    err = in_dev_dump_ifaddr(in_dev, skb, cb, &ctx->ip_idx,
> >                                             &fillargs);
> > else if (type == MULTICAST_ADDR)
> >    in_dev_dump_ifmcaddr(in_dev, skb, cb, s_ip_idx,
> >                                         &fillargs);
>
> The current functional call stack is as follows:
>
> inet_dump_ifaddr()/inet_dump_ifmcaddr() -> inet_dump_addr() ->
> in_dev_dump_ifaddr()/in_dev_dump_ifmcaddr().
>
> The ifaddr and ifmcaddr dump code paths share common logic inside
> inet_dump_addr(). If we don't do the dispatching in
> in_dev_dump_addr(), we have to do the dispatching in inet_dump_addr()
> instead, and the dispatching logic will be duplicated twice. I don't
> think this will simplify the code.
>
> Or do you suggest I should pass a function pointer for
> in_dev_dump_ifaddr()/in_dev_dump_ifmcaddr() into inet_dump_addr()?
>
> Thanks,
>
> Yuyang
>
> On Fri, Jan 10, 2025 at 12:33 AM David Ahern <dsahern@kernel.org> wrote:
> >
> > On 1/9/25 12:22 AM, Yuyang Huang wrote:
> > > @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
> > >       return res;
> > >  }
> > >
> > > -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> > > +                       enum addr_type_t type)
> > >  {
> > >       const struct nlmsghdr *nlh = cb->nlh;
> > >       struct inet_fill_args fillargs = {
> > >               .portid = NETLINK_CB(cb->skb).portid,
> > >               .seq = nlh->nlmsg_seq,
> > > -             .event = RTM_NEWADDR,
> > >               .flags = NLM_F_MULTI,
> > >               .netnsid = -1,
> > > +             .type = type,
> >
> > my comment meant that this `type` should be removed and the wrappers
> > below just call the intended function. No need for the extra layers.
> >
> > >       };
> > >       struct net *net = sock_net(skb->sk);
> > >       struct net *tgt_net = net;
> > > @@ -1949,6 +1996,20 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > >       return err;
> > >  }
> > >
> > > +static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > +{
> > > +     enum addr_type_t type = UNICAST_ADDR;
> > > +
> > > +     return inet_dump_addr(skb, cb, type);
> > > +}
> > > +
> > > +static int inet_dump_ifmcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > +{
> > > +     enum addr_type_t type = MULTICAST_ADDR;
> > > +
> > > +     return inet_dump_addr(skb, cb, type);
> > > +}
> > > +
> > >  static void rtmsg_ifa(int event, struct in_ifaddr *ifa, struct nlmsghdr *nlh,
> > >                     u32 portid)
> > >  {
> >

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[Yuyang Huang]

>You don't have to wait for iproute2 changes to be merged.
>Push them somewhere we can pull from, and when you post v5
>(with the self-test) just add a link to that iproute2 git repo
>to the cover letter.
>We'll pull your changes to the iproute2 used by the CI image.

Thanks for the suggestion, I will include the iproute2 based self-test
in v5 patch.

Thanks
Yuyang

On Fri, Jan 10, 2025 at 1:09 AM Yuyang Huang <yuyanghuang@google.com> wrote:
>
> >my comment meant that this `type` should be removed and the wrappers
> >below just call the intended function. No need for the extra layers.
>
> This patch was trying to follow the similar pattern in addrconf.c. We
> have a similar addr_type_t based dispatching mechanism to handle the
> dumping of ANYCAST, MULTICAST, and UNICAST addresses.
>
> inet6_dump_ifaddr()/inet6_dump_ifmcaddr()/inet6_dump_ifacaddr() ->
> inet6_dump_addr() -> in6_dump_addrs()
>
> The dispatching switch statement resides within in6_dump_addrs(), and
> those dump functions share the common code path in inet6_dump_addr().
>
> Thanks,
> Yuyang
>
>
> On Fri, Jan 10, 2025 at 12:52 AM Yuyang Huang <yuyanghuang@google.com> wrote:
> >
> > >my comment meant that this `type` should be removed and the wrappers
> > >below just call the intended function. No need for the extra layers.
> >
> > Sorry, I still do not fully understand the suggestions.
> >
> > In the current inet_dump_ifaddr() function, there are two places where
> > in_dev_dump_addr() is called.
> >
> > For example, we have the following code snippet.
> >
> > >if (!in_dev)
> > >goto done;
> > >err = in_dev_dump_addr(in_dev, skb, cb, &ctx->ip_idx,
> > >       &fillargs);
> > >goto done;
> > >}
> >
> > Do you suggest we do the following way?
> >
> > > If (type == UNICAST_ADDR)
> > >    err = in_dev_dump_ifaddr(in_dev, skb, cb, &ctx->ip_idx,
> > >                                             &fillargs);
> > > else if (type == MULTICAST_ADDR)
> > >    in_dev_dump_ifmcaddr(in_dev, skb, cb, s_ip_idx,
> > >                                         &fillargs);
> >
> > The current functional call stack is as follows:
> >
> > inet_dump_ifaddr()/inet_dump_ifmcaddr() -> inet_dump_addr() ->
> > in_dev_dump_ifaddr()/in_dev_dump_ifmcaddr().
> >
> > The ifaddr and ifmcaddr dump code paths share common logic inside
> > inet_dump_addr(). If we don't do the dispatching in
> > in_dev_dump_addr(), we have to do the dispatching in inet_dump_addr()
> > instead, and the dispatching logic will be duplicated twice. I don't
> > think this will simplify the code.
> >
> > Or do you suggest I should pass a function pointer for
> > in_dev_dump_ifaddr()/in_dev_dump_ifmcaddr() into inet_dump_addr()?
> >
> > Thanks,
> >
> > Yuyang
> >
> > On Fri, Jan 10, 2025 at 12:33 AM David Ahern <dsahern@kernel.org> wrote:
> > >
> > > On 1/9/25 12:22 AM, Yuyang Huang wrote:
> > > > @@ -1889,15 +1935,16 @@ static u32 inet_base_seq(const struct net *net)
> > > >       return res;
> > > >  }
> > > >
> > > > -static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > > +static int inet_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
> > > > +                       enum addr_type_t type)
> > > >  {
> > > >       const struct nlmsghdr *nlh = cb->nlh;
> > > >       struct inet_fill_args fillargs = {
> > > >               .portid = NETLINK_CB(cb->skb).portid,
> > > >               .seq = nlh->nlmsg_seq,
> > > > -             .event = RTM_NEWADDR,
> > > >               .flags = NLM_F_MULTI,
> > > >               .netnsid = -1,
> > > > +             .type = type,
> > >
> > > my comment meant that this `type` should be removed and the wrappers
> > > below just call the intended function. No need for the extra layers.
> > >
> > > >       };
> > > >       struct net *net = sock_net(skb->sk);
> > > >       struct net *tgt_net = net;
> > > > @@ -1949,6 +1996,20 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > >       return err;
> > > >  }
> > > >
> > > > +static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > > +{
> > > > +     enum addr_type_t type = UNICAST_ADDR;
> > > > +
> > > > +     return inet_dump_addr(skb, cb, type);
> > > > +}
> > > > +
> > > > +static int inet_dump_ifmcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > > > +{
> > > > +     enum addr_type_t type = MULTICAST_ADDR;
> > > > +
> > > > +     return inet_dump_addr(skb, cb, type);
> > > > +}
> > > > +
> > > >  static void rtmsg_ifa(int event, struct in_ifaddr *ifa, struct nlmsghdr *nlh,
> > > >                     u32 portid)
> > > >  {
> > >

Re: [PATCH net-next, v4] netlink: support dumping IPv4 multicast addresses

[David Ahern]

On 1/9/25 8:52 AM, Yuyang Huang wrote:
>> my comment meant that this `type` should be removed and the wrappers
>> below just call the intended function. No need for the extra layers.
> 
> Sorry, I still do not fully understand the suggestions.
> 
> In the current inet_dump_ifaddr() function, there are two places where
> in_dev_dump_addr() is called.
> 

nevermind. I forgot how IPv6 address dumps are coded, and keeping the 2
protocols aligned is best. So, I guess you just need to resolve the
failures Paolo noted.