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

Hi Paolo

Thanks for the review feedback, I will adjust them in the patch v6.

Why moving the struct definition here? IMHO addrconf.h is better suited and will avoid additional headers dep.

The `struct inet_fill_args` is moved from devinet.c to igmp.h to make it usable in both devinet.c and igmp.c. I feel it is incorrect to move `struct inet_fill_args` to addrconf.h because that file contains IPv6 specific definition and it also contains `struct inet6_fill_args`. After refactoring, I will remove the usage of enum addr_type_t type, so we don't need to import addrconf.h anymore.

Thanks,

Yuyang

On Thu, Jan 16, 2025 at 8:06 PM Paolo Abeni pabeni@redhat.com wrote:

On 1/14/25 3:37 AM, Yuyang Huang wrote:

...

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.

This change includes a new ynl based selftest case. To run the test, execute the following command:

$ vng -v --user root --cpus 16 -- \ make -C tools/testing/selftests TARGETS=net \ TEST_PROGS=rtnetlink.py TEST_GEN_PROGS="" run_tests

Thanks for including the test!

...

diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml index 0d492500c7e5..7dcd5fddac9d 100644 --- a/Documentation/netlink/specs/rt_link.yaml +++ b/Documentation/netlink/specs/rt_link.yaml @@ -92,6 +92,41 @@ definitions: - name: ifi-change type: u32

• name: ifaddrmsg
• type: struct
• members:

•  -
  

•    name: ifa-family
  

•    type: u8
  

•  -
  

•    name: ifa-prefixlen
  

•    type: u8
  

•  -
  

•    name: ifa-flags
  

•    type: u8
  

•  -
  

•    name: ifa-scope
  

•    type: u8
  

•  -
  

•    name: ifa-index
  

•    type: u32
  

• name: ifacacheinfo
• type: struct
• members:

•  -
  

•    name: ifa-prefered
  

•    type: u32
  

•  -
  

•    name: ifa-valid
  

•    type: u32
  

•  -
  

•    name: cstamp
  

•    type: u32
  

•  -
  

•    name: tstamp
  

•    type: u32
  

  • name: ifla-bridge-id type: struct

@@ -2253,6 +2288,18 @@ attribute-sets: - name: tailroom type: u16

• name: ifmcaddr-attrs
• attributes:

•  -
  

•    name: addr
  

•    type: binary
  

•    value: 7
  

•  -
  

•    name: cacheinfo
  

•    type: binary
  

•    struct: ifacacheinfo
  

•    value: 6
  
  

sub-messages:

@@ -2493,6 +2540,29 @@ operations: reply: value: 92 attributes: *link-stats-attrs

•  name: getmaddrs
  

•  doc: Get / dump IPv4/IPv6 multicast addresses.
  

•  attribute-set: ifmcaddr-attrs
  

•  fixed-header: ifaddrmsg
  

•  do:
  

•    request:
  

•      value: 58
  

•      attributes:
  

•        - ifa-family
  

•        - ifa-index
  

•    reply:
  

•      value: 58
  

•      attributes: &mcaddr-attrs
  

•        - addr
  

•        - cacheinfo
  

•  dump:
  

•    request:
  

•      value: 58
  

•        - ifa-family
  

•    reply:
  

•      value: 58
  

•      attributes: *mcaddr-attrs
  
  

mcast-groups: list:

IMHO the test case itself should land to a separate patch.

...

diff --git a/include/linux/igmp.h b/include/linux/igmp.h index 073b30a9b850..a460e1ef0524 100644 --- a/include/linux/igmp.h +++ b/include/linux/igmp.h @@ -16,6 +16,7 @@ #include <linux/ip.h> #include <linux/refcount.h> #include <linux/sockptr.h> +#include <net/addrconf.h> #include <uapi/linux/igmp.h>

static inline struct igmphdr *igmp_hdr(const struct sk_buff *skb) @@ -92,6 +93,16 @@ struct ip_mc_list { struct rcu_head rcu; };

+struct inet_fill_args {

• u32 portid;
  

• u32 seq;
  

• int event;
  

• unsigned int flags;
  

• int netnsid;
  

• int ifindex;
  

• enum addr_type_t type;
  

+};

Why moving the struct definition here? IMHO addrconf.h is better suited and will avoid additional headers dep.

...

@@ -1874,6 +1894,23 @@ 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:
  

•         fillargs->event = RTM_NEWADDR;
  

I think that adding an additional argument for 'event' to inet_dump_addr() would simplify the code a bit.

...

•         return in_dev_dump_ifaddr(in_dev, skb, cb, s_ip_idx, fillargs);
  

• case MULTICAST_ADDR:
  

•         fillargs->event = RTM_GETMULTICAST;
  

•         return in_dev_dump_ifmcaddr(in_dev, skb, cb, s_ip_idx,
  

•                                     fillargs);
  

• default:
  

•         return 0;
  

Why not erroring out on unknown types? should never happen, right?

...

@@ -1949,6 +1987,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);
  

You can avoid the local variable usage.

...

+}

+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);
  

Same here.

Thanks!

Paolo