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

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <1708531057-67392-2-git-send-email-jdamato@fastly.com>
References: <1708531057-67392-2-git-send-email-jdamato@fastly.com>
TO: Joe Damato <jdamato@fastly.com>
TO: netdev@vger.kernel.org
TO: linux-kernel@vger.kernel.org
CC: Joe Damato <jdamato@fastly.com>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Jesper Dangaard Brouer <hawk@kernel.org>
CC: Stanislav Fomichev <sdf@google.com>
CC: Amritha Nambiar <amritha.nambiar@intel.com>
CC: Larysa Zaremba <larysa.zaremba@intel.com>
CC: Lorenzo Bianconi <lorenzo@kernel.org>
CC: Tariq Toukan <tariqt@nvidia.com>
CC: Sridhar Samudrala <sridhar.samudrala@intel.com>
CC: Alexei Starovoitov <ast@kernel.org>
CC: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Hi Joe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/netdev-genl-Add-ifname-for-queue-and-NAPI-APIs/20240222-000134
base:   net-next/main
patch link:    https://lore.kernel.org/r/1708531057-67392-2-git-send-email-jdamato%40fastly.com
patch subject: [PATCH net-next 1/2] netdev-genl: Add ifname for queue and NAPI APIs
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: i386-randconfig-141-20240222 (https://download.01.org/0day-ci/archive/20240223/202402231851.2NeORqwi-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202402231851.2NeORqwi-lkp@intel.com/

New smatch warnings:
net/core/netdev-genl.c:388 netdev_nl_queue_get_doit() error: uninitialized symbol 'ifname'.

Old smatch warnings:
net/core/netdev-genl.c:45 netdev_nl_dev_fill() warn: inconsistent indenting

vim +/ifname +388 net/core/netdev-genl.c

6b6171db7fc8f7 Amritha Nambiar 2023-12-01  370  
bc877956272f05 Amritha Nambiar 2023-12-01  371  int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
bc877956272f05 Amritha Nambiar 2023-12-01  372  {
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  373  	u32 q_id, q_type, ifindex;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  374  	struct net_device *netdev;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  375  	struct sk_buff *rsp;
f340b224321fa6 Joe Damato      2024-02-21  376  	char *ifname;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  377  	int err;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  378  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  379  	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  380  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
f340b224321fa6 Joe Damato      2024-02-21  381  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX) ||
f340b224321fa6 Joe Damato      2024-02-21  382  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFNAME))
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  383  		return -EINVAL;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  384  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  385  	q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  386  	q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  387  	ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
f340b224321fa6 Joe Damato      2024-02-21 @388  	nla_strscpy(ifname, info->attrs[NETDEV_A_QUEUE_IFNAME], IFNAMSIZ);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  389  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  390  	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  391  	if (!rsp)
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  392  		return -ENOMEM;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  393  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  394  	rtnl_lock();
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  395  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  396  	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
f340b224321fa6 Joe Damato      2024-02-21  397  
f340b224321fa6 Joe Damato      2024-02-21  398  	if (strcmp(netdev->name, ifname)) {
f340b224321fa6 Joe Damato      2024-02-21  399  		err = -ENODEV;
f340b224321fa6 Joe Damato      2024-02-21  400  	} else {
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  401  		if (netdev)
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  402  			err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  403  		else
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  404  			err = -ENODEV;
f340b224321fa6 Joe Damato      2024-02-21  405  	}
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  406  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  407  	rtnl_unlock();
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  408  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  409  	if (err)
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  410  		goto err_free_msg;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  411  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  412  	return genlmsg_reply(rsp, info);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  413  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  414  err_free_msg:
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  415  	nlmsg_free(rsp);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  416  	return err;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  417  }
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  418  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next 0/2] Expose netdev name in netdev netlink APIs

[Joe Damato]

Greetings:

The netdev netlink APIs currently provide the ifindex of a device
associated with the NIC queue or NAPI when the netlink API is used. In
order for user applications to map this back to a human readable device
name, user applications must issue a subsequent ioctl (SIOCGIFNAME) in
order to map an ifindex back to a device name.

This patch set adds ifname to the API so that when queue or NAPI
information is retrieved, the human readable string is included. The netdev
netlink YAML spec has been updated to include this field, as well.

This saves the subsequent call to ioctl and makes the netlink information
more user friendly. Applications might use this information in conjunction
with SO_INCOMING_NAPI_ID to map NAPI IDs to specific NICs with application
specific configuration (e.g. NUMA zone and CPU layout information).

An example using the netlink cli tool before this change:

$ ./cli.py --spec netdev.yaml --dump queue-get --json='{"ifindex": 7}'
[{'id': 0, 'ifindex': 7, 'type': 'rx'},
 {'id': 1, 'ifindex': 7, 'type': 'rx'},
 {'id': 2, 'ifindex': 7, 'type': 'rx'},
...

$ ./cli.py --spec netdev.yaml --dump napi-get --json='{"ifindex": 7}'
[{'id': 448, 'ifindex': 7},
 {'id': 447, 'ifindex': 7},
 {'id': 446, 'ifindex': 7},
...

An example after this change:

$ ./cli.py --spec netdev.yaml --dump queue-get --json='{"ifindex": 7}'
[{'id': 0, 'ifindex': 7, 'ifname': 'eth1', 'type': 'rx'},
 {'id': 1, 'ifindex': 7, 'ifname': 'eth1', 'type': 'rx'},
 {'id': 2, 'ifindex': 7, 'ifname': 'eth1', 'type': 'rx'},
 ...

$ ./cli.py --spec netdev.yaml --dump napi-get --json='{"ifindex": 7}'
[{'id': 448, 'ifindex': 7, 'ifname': 'eth1'},
 {'id': 447, 'ifindex': 7, 'ifname': 'eth1'},
 {'id': 446, 'ifindex': 7, 'ifname': 'eth1'},
 ...

Thanks,
Joe

Joe Damato (2):
  netdev-genl: Add ifname for queue and NAPI APIs
  netdev-genl: spec: Add ifname to netdev nl YAML spec

 Documentation/netlink/specs/netdev.yaml | 10 ++++++++++
 include/uapi/linux/netdev.h             |  2 ++
 net/core/netdev-genl.c                  | 22 +++++++++++++++++-----
 tools/include/uapi/linux/netdev.h       |  2 ++
 4 files changed, 31 insertions(+), 5 deletions(-)

-- 
2.7.4

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

[Joe Damato]

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;
+	}
 
 	rtnl_unlock();
 
-- 
2.7.4

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

[Jakub Kicinski]

On Wed, 21 Feb 2024 07:57:29 -0800 Joe Damato wrote:
>  	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))

This means user always has to provide both ifindex and ifname,
right?

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

[Joe Damato]

On Wed, Feb 21, 2024 at 11:12:20AM -0800, Jakub Kicinski wrote:
> On Wed, 21 Feb 2024 07:57:29 -0800 Joe Damato wrote:
> >  	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))
> 
> This means user always has to provide both ifindex and ifname,
> right?

That's right. I'm OK with omitting this requirement, though. I feel like to
your earlier point on name changes, maybe ifindex is enough as far as
required params go.

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

[Nambiar, Amritha]

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;

>   	rtnl_unlock();
>   

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

[Joe Damato]

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.

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

[kernel test robot]

Hi Joe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/netdev-genl-Add-ifname-for-queue-and-NAPI-APIs/20240222-000134
base:   net-next/main
patch link:    https://lore.kernel.org/r/1708531057-67392-2-git-send-email-jdamato%40fastly.com
patch subject: [PATCH net-next 1/2] netdev-genl: Add ifname for queue and NAPI APIs
config: powerpc-acadia_defconfig (https://download.01.org/0day-ci/archive/20240223/202402230429.g2qC2zMK-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project edd4aee4dd9b5b98b2576a6f783e4086173d902a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240223/202402230429.g2qC2zMK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402230429.g2qC2zMK-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/core/netdev-genl.c:388:14: warning: variable 'ifname' is uninitialized when used here [-Wuninitialized]
     388 |         nla_strscpy(ifname, info->attrs[NETDEV_A_QUEUE_IFNAME], IFNAMSIZ);
         |                     ^~~~~~
   net/core/netdev-genl.c:376:14: note: initialize the variable 'ifname' to silence this warning
     376 |         char *ifname;
         |                     ^
         |                      = NULL
   1 warning generated.


vim +/ifname +388 net/core/netdev-genl.c

   370	
   371	int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
   372	{
   373		u32 q_id, q_type, ifindex;
   374		struct net_device *netdev;
   375		struct sk_buff *rsp;
   376		char *ifname;
   377		int err;
   378	
   379		if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
   380		    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
   381		    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX) ||
   382		    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFNAME))
   383			return -EINVAL;
   384	
   385		q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]);
   386		q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]);
   387		ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
 > 388		nla_strscpy(ifname, info->attrs[NETDEV_A_QUEUE_IFNAME], IFNAMSIZ);
   389	
   390		rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
   391		if (!rsp)
   392			return -ENOMEM;
   393	
   394		rtnl_lock();
   395	
   396		netdev = __dev_get_by_index(genl_info_net(info), ifindex);
   397	
   398		if (strcmp(netdev->name, ifname)) {
   399			err = -ENODEV;
   400		} else {
   401			if (netdev)
   402				err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
   403			else
   404				err = -ENODEV;
   405		}
   406	
   407		rtnl_unlock();
   408	
   409		if (err)
   410			goto err_free_msg;
   411	
   412		return genlmsg_reply(rsp, info);
   413	
   414	err_free_msg:
   415		nlmsg_free(rsp);
   416		return err;
   417	}
   418	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Dan Carpenter]

Hi Joe,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/netdev-genl-Add-ifname-for-queue-and-NAPI-APIs/20240222-000134
base:   net-next/main
patch link:    https://lore.kernel.org/r/1708531057-67392-2-git-send-email-jdamato%40fastly.com
patch subject: [PATCH net-next 1/2] netdev-genl: Add ifname for queue and NAPI APIs
config: i386-randconfig-141-20240222 (https://download.01.org/0day-ci/archive/20240223/202402231851.2NeORqwi-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202402231851.2NeORqwi-lkp@intel.com/

New smatch warnings:
net/core/netdev-genl.c:388 netdev_nl_queue_get_doit() error: uninitialized symbol 'ifname'.

vim +/ifname +388 net/core/netdev-genl.c

bc877956272f05 Amritha Nambiar 2023-12-01  371  int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
bc877956272f05 Amritha Nambiar 2023-12-01  372  {
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  373  	u32 q_id, q_type, ifindex;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  374  	struct net_device *netdev;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  375  	struct sk_buff *rsp;
f340b224321fa6 Joe Damato      2024-02-21  376  	char *ifname;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  377  	int err;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  378  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  379  	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  380  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
f340b224321fa6 Joe Damato      2024-02-21  381  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX) ||
f340b224321fa6 Joe Damato      2024-02-21  382  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFNAME))
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  383  		return -EINVAL;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  384  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  385  	q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  386  	q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  387  	ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
f340b224321fa6 Joe Damato      2024-02-21 @388  	nla_strscpy(ifname, info->attrs[NETDEV_A_QUEUE_IFNAME], IFNAMSIZ);
                                                                    ^^^^^^
missing initialization

6b6171db7fc8f7 Amritha Nambiar 2023-12-01  389  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  390  	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  391  	if (!rsp)
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  392  		return -ENOMEM;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  393  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  394  	rtnl_lock();
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  395  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  396  	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
f340b224321fa6 Joe Damato      2024-02-21  397  
f340b224321fa6 Joe Damato      2024-02-21  398  	if (strcmp(netdev->name, ifname)) {
f340b224321fa6 Joe Damato      2024-02-21  399  		err = -ENODEV;
f340b224321fa6 Joe Damato      2024-02-21  400  	} else {
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  401  		if (netdev)
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  402  			err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  403  		else
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  404  			err = -ENODEV;
f340b224321fa6 Joe Damato      2024-02-21  405  	}
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  406  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  407  	rtnl_unlock();
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  408  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  409  	if (err)
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  410  		goto err_free_msg;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  411  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  412  	return genlmsg_reply(rsp, info);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  413  
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  414  err_free_msg:
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  415  	nlmsg_free(rsp);
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  416  	return err;
6b6171db7fc8f7 Amritha Nambiar 2023-12-01  417  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki