| Message ID | 20250109072245.2928832-1-yuyanghuang@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] netlink: support dumping IPv4 multicast addresses | expand |
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
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 >
>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 > >
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
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) > {
>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) > > { >
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.
>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) > > > { > >
>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) > > > > { > > >
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.
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);
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(-)