[PATCH net-next v2] mctp: emit RTM_NEWADDR and RTM_DELADDR

[PATCH net-next v2] mctp: emit RTM_NEWADDR and RTM_DELADDR

[Matt Johnston]

Userspace can receive notification of MCTP address changes via
RTNLGRP_MCTP_IFADDR rtnetlink multicast group.

Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---

v2: Simplify error return path, fix local variable ordering

I've left req_skb as-is rather than passing portid, as noted 
it keeps callers tidier.

 include/uapi/linux/rtnetlink.h |  2 ++
 net/mctp/device.c              | 51 ++++++++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..93d934cc4613 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -754,6 +754,8 @@ enum rtnetlink_groups {
 #define RTNLGRP_NEXTHOP		RTNLGRP_NEXTHOP
 	RTNLGRP_BRVLAN,
 #define RTNLGRP_BRVLAN		RTNLGRP_BRVLAN
+	RTNLGRP_MCTP_IFADDR,
+#define RTNLGRP_MCTP_IFADDR	RTNLGRP_MCTP_IFADDR
 	__RTNLGRP_MAX
 };
 #define RTNLGRP_MAX	(__RTNLGRP_MAX - 1)
diff --git a/net/mctp/device.c b/net/mctp/device.c
index 8799ee77e7b7..5790014ad825 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -35,14 +35,24 @@ struct mctp_dev *mctp_dev_get_rtnl(const struct net_device *dev)
 	return rtnl_dereference(dev->mctp_ptr);
 }
 
-static int mctp_fill_addrinfo(struct sk_buff *skb, struct netlink_callback *cb,
-			      struct mctp_dev *mdev, mctp_eid_t eid)
+static int mctp_addrinfo_size(void)
+{
+	return NLMSG_ALIGN(sizeof(struct ifaddrmsg))
+		+ nla_total_size(4) // IFA_LOCAL
+		+ nla_total_size(4) // IFA_ADDRESS
+		;
+}
+
+/* flag should be NLM_F_MULTI for dump calls */
+static int mctp_fill_addrinfo(struct sk_buff *skb,
+			      struct mctp_dev *mdev, mctp_eid_t eid,
+			      int msg_type, u32 portid, u32 seq, int flag)
 {
 	struct ifaddrmsg *hdr;
 	struct nlmsghdr *nlh;
 
-	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
-			RTM_NEWADDR, sizeof(*hdr), NLM_F_MULTI);
+	nlh = nlmsg_put(skb, portid, seq,
+			msg_type, sizeof(*hdr), flag);
 	if (!nlh)
 		return -EMSGSIZE;
 
@@ -72,10 +82,14 @@ static int mctp_dump_dev_addrinfo(struct mctp_dev *mdev, struct sk_buff *skb,
 				  struct netlink_callback *cb)
 {
 	struct mctp_dump_cb *mcb = (void *)cb->ctx;
+	u32 portid, seq;
 	int rc = 0;
 
+	portid = NETLINK_CB(cb->skb).portid;
+	seq = cb->nlh->nlmsg_seq;
 	for (; mcb->a_idx < mdev->num_addrs; mcb->a_idx++) {
-		rc = mctp_fill_addrinfo(skb, cb, mdev, mdev->addrs[mcb->a_idx]);
+		rc = mctp_fill_addrinfo(skb, mdev, mdev->addrs[mcb->a_idx],
+					RTM_NEWADDR, portid, seq, NLM_F_MULTI);
 		if (rc < 0)
 			break;
 	}
@@ -127,6 +141,30 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+static void mctp_addr_notify(struct mctp_dev *mdev, mctp_eid_t eid, int msg_type,
+			     struct sk_buff *req_skb, struct nlmsghdr *req_nlh)
+{
+	u32 portid = NETLINK_CB(req_skb).portid;
+	struct net *net = dev_net(mdev->dev);
+	struct sk_buff *skb;
+	int rc = -ENOBUFS;
+
+	skb = nlmsg_new(mctp_addrinfo_size(), GFP_KERNEL);
+	if (!skb)
+		goto out;
+
+	rc = mctp_fill_addrinfo(skb, mdev, eid, msg_type,
+				portid, req_nlh->nlmsg_seq, 0);
+	if (rc < 0)
+		goto out;
+
+	rtnl_notify(skb, net, portid, RTNLGRP_MCTP_IFADDR, req_nlh, GFP_KERNEL);
+	return;
+out:
+	kfree_skb(skb);
+	rtnl_set_sk_err(net, RTNLGRP_MCTP_IFADDR, rc);
+}
+
 static const struct nla_policy ifa_mctp_policy[IFA_MAX + 1] = {
 	[IFA_ADDRESS]		= { .type = NLA_U8 },
 	[IFA_LOCAL]		= { .type = NLA_U8 },
@@ -189,6 +227,7 @@ static int mctp_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	kfree(tmp_addrs);
 
+	mctp_addr_notify(mdev, addr->s_addr, RTM_NEWADDR, skb, nlh);
 	mctp_route_add_local(mdev, addr->s_addr);
 
 	return 0;
@@ -244,6 +283,8 @@ static int mctp_rtm_deladdr(struct sk_buff *skb, struct nlmsghdr *nlh,
 	mdev->num_addrs--;
 	spin_unlock_irqrestore(&mdev->addrs_lock, flags);
 
+	mctp_addr_notify(mdev, addr->s_addr, RTM_DELADDR, skb, nlh);
+
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH net-next v2] mctp: emit RTM_NEWADDR and RTM_DELADDR

[Jakub Kicinski]

On Wed, 15 Dec 2021 13:32:50 +0800 Matt Johnston wrote:
> Userspace can receive notification of MCTP address changes via
> RTNLGRP_MCTP_IFADDR rtnetlink multicast group.
> 
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
> 
> v2: Simplify error return path, fix local variable ordering
> 
> I've left req_skb as-is rather than passing portid, as noted 
> it keeps callers tidier.

Sorry for late review.

> -static int mctp_fill_addrinfo(struct sk_buff *skb, struct netlink_callback *cb,
> -			      struct mctp_dev *mdev, mctp_eid_t eid)
> +static int mctp_addrinfo_size(void)
> +{
> +	return NLMSG_ALIGN(sizeof(struct ifaddrmsg))
> +		+ nla_total_size(4) // IFA_LOCAL
> +		+ nla_total_size(4) // IFA_ADDRESS

why 4? The attributes are u8, no?

> +		;
> +}

> @@ -127,6 +141,30 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +static void mctp_addr_notify(struct mctp_dev *mdev, mctp_eid_t eid, int msg_type,
> +			     struct sk_buff *req_skb, struct nlmsghdr *req_nlh)
> +{
> +	u32 portid = NETLINK_CB(req_skb).portid;
> +	struct net *net = dev_net(mdev->dev);
> +	struct sk_buff *skb;
> +	int rc = -ENOBUFS;
> +
> +	skb = nlmsg_new(mctp_addrinfo_size(), GFP_KERNEL);
> +	if (!skb)
> +		goto out;
> +
> +	rc = mctp_fill_addrinfo(skb, mdev, eid, msg_type,
> +				portid, req_nlh->nlmsg_seq, 0);
> +	if (rc < 0)

How about a customary WARN_ON_ONCE(rc == -EMSGSIZE) here?

> +		goto out;
> +
> +	rtnl_notify(skb, net, portid, RTNLGRP_MCTP_IFADDR, req_nlh, GFP_KERNEL);
> +	return;
> +out:
> +	kfree_skb(skb);
> +	rtnl_set_sk_err(net, RTNLGRP_MCTP_IFADDR, rc);
> +}

Re: [PATCH net-next v2] mctp: emit RTM_NEWADDR and RTM_DELADDR

[Matt Johnston]

On Fri, 2021-12-17 at 11:42 -0800, Jakub Kicinski wrote:
> 	return NLMSG_ALIGN(sizeof(struct ifaddrmsg))
> > +		+ nla_total_size(4) // IFA_LOCAL
> > +		+ nla_total_size(4) // IFA_ADDRESS
> 
> why 4? The attributes are u8, no?
> 
> 
> > +	if (rc < 0)
> How about a customary WARN_ON_ONCE(rc == -EMSGSIZE) here?

Thanks for the review Jakub. Size 4 is a mistake, I'll send v3.

Cheers,
Matt
>