Re: [PATCH net-next 1/2] netdev-genl: Add ifname for queue and NAPI APIs

[Joe Damato <jdamato@fastly.com>]

On Wed, Feb 21, 2024 at 11:12:47AM -0800, Nambiar, Amritha wrote:
> On 2/21/2024 7:57 AM, Joe Damato wrote:
> >Expose the netdevice name when queue and NAPI netdev-genl APIs are used
> >
> >Signed-off-by: Joe Damato <jdamato@fastly.com>
> >---
> >  include/uapi/linux/netdev.h |  2 ++
> >  net/core/netdev-genl.c      | 22 +++++++++++++++++-----
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> >
> >diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> >index 93cb411..80762bc 100644
> >--- a/include/uapi/linux/netdev.h
> >+++ b/include/uapi/linux/netdev.h
> >@@ -117,6 +117,7 @@ enum {
> >  	NETDEV_A_NAPI_ID,
> >  	NETDEV_A_NAPI_IRQ,
> >  	NETDEV_A_NAPI_PID,
> >+	NETDEV_A_NAPI_IFNAME,
> >  	__NETDEV_A_NAPI_MAX,
> >  	NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
> >@@ -127,6 +128,7 @@ enum {
> >  	NETDEV_A_QUEUE_IFINDEX,
> >  	NETDEV_A_QUEUE_TYPE,
> >  	NETDEV_A_QUEUE_NAPI_ID,
> >+	NETDEV_A_QUEUE_IFNAME,
> >  	__NETDEV_A_QUEUE_MAX,
> >  	NETDEV_A_QUEUE_MAX = (__NETDEV_A_QUEUE_MAX - 1)
> >diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> >index fd98936..a886e6a 100644
> >--- a/net/core/netdev-genl.c
> >+++ b/net/core/netdev-genl.c
> >@@ -181,6 +181,9 @@ netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
> >  	if (nla_put_u32(rsp, NETDEV_A_NAPI_IFINDEX, napi->dev->ifindex))
> >  		goto nla_put_failure;
> >+	if (nla_put_string(rsp, NETDEV_A_NAPI_IFNAME, napi->dev->name))
> >+		goto nla_put_failure;
> >+
> >  	if (napi->irq >= 0 && nla_put_u32(rsp, NETDEV_A_NAPI_IRQ, napi->irq))
> >  		goto nla_put_failure;
> >@@ -307,7 +310,8 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
> >  	if (nla_put_u32(rsp, NETDEV_A_QUEUE_ID, q_idx) ||
> >  	    nla_put_u32(rsp, NETDEV_A_QUEUE_TYPE, q_type) ||
> >-	    nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex))
> >+	    nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex) ||
> >+	    nla_put_string(rsp, NETDEV_A_QUEUE_IFNAME, netdev->name))
> >  		goto nla_put_failure;
> >  	switch (q_type) {
> >@@ -369,16 +373,19 @@ int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
> >  	u32 q_id, q_type, ifindex;
> >  	struct net_device *netdev;
> >  	struct sk_buff *rsp;
> >+	char *ifname;
> >  	int err;
> >  	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
> >  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
> >-	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX))
> >+	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX) ||
> >+	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFNAME))
> >  		return -EINVAL;
> >  	q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]);
> >  	q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]);
> >  	ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
> >+	nla_strscpy(ifname, info->attrs[NETDEV_A_QUEUE_IFNAME], IFNAMSIZ);
> >  	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >  	if (!rsp)
> >@@ -387,10 +394,15 @@ int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
> >  	rtnl_lock();
> >  	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> >-	if (netdev)
> >-		err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
> >-	else
> >+
> >+	if (strcmp(netdev->name, ifname)) {
> >  		err = -ENODEV;
> >+	} else {
> >+		if (netdev)
> >+			err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
> >+		else
> >+			err = -ENODEV;
> >+	}
> 
> This looks bit incorrect to me that the netdev is checked after netdev->name
> is accessed. Shouldn't this be something like:
> 
> if (netdev && !strcmp(netdev->name, ifname))
> 	err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
> else
> 	err = -ENODEV;

Yes, you are right. Thanks.

Based on Jakub's comment re exposing names, though, it seems that perhaps
this change is not desirable overall.