| 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
| ||
|
Message-ID: <b7305f0a-fe4d-4dd4-aaaf-77a08fd919ac@redhat.com>
Date: Thu, 16 Jan 2025 12:06:48 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Yuyang Huang <yuyanghuang@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
netdev@...r.kernel.org, Donald Hunter <donald.hunter@...il.com>,
Shuah Khan <shuah@...nel.org>, Nikolay Aleksandrov <razor@...ckwall.org>,
Hangbin Liu <liuhangbin@...il.com>, Daniel Borkmann <daniel@...earbox.net>,
linux-kselftest@...r.kernel.org, Maciej Żenczykowski
<maze@...gle.com>, Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [PATCH net-next, v5] netlink: support dumping IPv4 multicast
addresses
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
Powered by blists - more mailing lists