[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d34d621-421b-40bd-98ab-c93783408d74@intel.com>
Date: Wed, 21 Feb 2024 11:12:47 -0800
From: "Nambiar, Amritha" <amritha.nambiar@...el.com>
To: Joe Damato <jdamato@...tly.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Jesper Dangaard Brouer <hawk@...nel.org>, "Stanislav
Fomichev" <sdf@...gle.com>, Larysa Zaremba <larysa.zaremba@...el.com>,
"Lorenzo Bianconi" <lorenzo@...nel.org>, Tariq Toukan <tariqt@...dia.com>,
"Sridhar Samudrala" <sridhar.samudrala@...el.com>, Alexei Starovoitov
<ast@...nel.org>, Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Subject: Re: [PATCH net-next 1/2] netdev-genl: Add ifname for queue and NAPI
APIs
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@...tly.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;
> rtnl_unlock();
>
Powered by blists - more mailing lists