Re: [PATCH net-next, v5] netlink: add IGMP/MLD join/leave notifications

[Jakub Kicinski <kuba@kernel.org>]

On Fri,  6 Dec 2024 13:10:25 +0900 Yuyang Huang wrote:
> +static int inet6_fill_ifmcaddr(struct sk_buff *skb, struct net_device *dev,
> +			       const struct in6_addr *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_INET6;
> +	ifm->ifa_prefixlen = 128;
> +	ifm->ifa_flags = IFA_F_PERMANENT;
> +	ifm->ifa_scope = RT_SCOPE_UNIVERSE;
> +	ifm->ifa_index = dev->ifindex;
> +
> +	if (nla_put_in6_addr(skb, IFA_MULTICAST, addr) < 0) {
> +		nlmsg_cancel(skb, nlh);
> +		return -EMSGSIZE;
> +	}
> +
> +	nlmsg_end(skb, nlh);
> +	return 0;
> +}

Is there a strong reason you reimplement this instead of trying to reuse
inet6_fill_ifmcaddr() ? Keeping notifications and get responses in sync
used to be a thing in rtnetlink, this code already diverged but maybe
we can bring it back.

> +static void inet6_ifmcaddr_notify(struct net_device *dev,
> +				  const struct in6_addr *addr, int event)
> +{
> +	struct net *net = dev_net(dev);
> +	struct sk_buff *skb;
> +	int err = -ENOBUFS;

ENOMEM ? I could be wrong but in atomic context the memory pressure 
can well be transient, it's not like the socket queue filled up.

> +	skb = nlmsg_new(NLMSG_ALIGN(sizeof(struct ifaddrmsg))
> +			+ nla_total_size(16), GFP_ATOMIC);

nit: + goes to the end of previous line

> +	if (!skb)
> +		goto error;
> +
> +	err = inet6_fill_ifmcaddr(skb, dev, addr, event);
> +	if (err < 0) {
> +		WARN_ON_ONCE(err == -EMSGSIZE);
> +		kfree_skb(skb);

nit: nlmsg_free(), since it exists

> +		goto error;
> +	}
> +
> +	rtnl_notify(skb, net, 0, RTNLGRP_IPV6_MCADDR, NULL, GFP_ATOMIC);
> +	return;
> +error:
> +	rtnl_set_sk_err(net, RTNLGRP_IPV6_MCADDR, err);
> +}
> +
-- 
pw-bot: cr