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

[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

[PATCH net-next 2/2] netdev-genl: spec: Add ifname to netdev nl YAML spec

[Joe Damato]

Add support to netdev netlink spec (netdev.yaml) for the ifname of the net
device.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 Documentation/netlink/specs/netdev.yaml | 10 ++++++++++
 tools/include/uapi/linux/netdev.h       |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index 3addac9..9a92d04 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -240,6 +240,10 @@ attribute-sets:
              threaded mode. If NAPI is not in threaded mode (i.e. uses normal
              softirq context), the attribute will be absent.
         type: u32
+      -
+        name: ifname
+        doc: name of the netdevice to which NAPI instance belongs.
+        type: string
   -
     name: queue
     attributes:
@@ -264,6 +268,10 @@ attribute-sets:
         name: napi-id
         doc: ID of the NAPI instance which services this queue.
         type: u32
+      -
+        name: ifname
+        doc: name of the netdevice to which the queue belongs.
+        type: string
 
 operations:
   list:
@@ -381,6 +389,7 @@ operations:
             - type
             - napi-id
             - ifindex
+            - ifname
       dump:
         request:
           attributes:
@@ -400,6 +409,7 @@ operations:
             - ifindex
             - irq
             - pid
+            - ifname
       dump:
         request:
           attributes:
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index 93cb411..80762bc 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/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)
-- 
2.7.4

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

[Jakub Kicinski]

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

To be clear, if_indextoname() is doing it, right? I wanted to be sure
the concern is really number of syscalls, not the difficulty in getting
the 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).

For context, the reason why I left the names out is that they can change
at any moment, but primarily because there are also altnames now:

2: eth0:
[...]
    altname enp2s0np0

Most of the APIs try to accept altnames as well as the "main" name.
If we propagate the name we'll step back into the rtnetlink naming
mess :(

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

[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: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 0/2] Expose netdev name in netdev netlink APIs

[Joe Damato]

On Wed, Feb 21, 2024 at 11:09:52AM -0800, Jakub Kicinski wrote:
> On Wed, 21 Feb 2024 07:57:28 -0800 Joe Damato wrote:
> > 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.
> 
> To be clear, if_indextoname() is doing it, right? I wanted to be sure
> the concern is really number of syscalls, not the difficulty in getting
> the name.

It seemed a bit odd to me to require the user to hit different APIs -- one
to get the ifindex and then another to get the name. I didn't realize you
had intentionally left the name out, though.

> > 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).
> 
> For context, the reason why I left the names out is that they can change
> at any moment, but primarily because there are also altnames now:
> 
> 2: eth0:
> [...]
>     altname enp2s0np0
> 
> Most of the APIs try to accept altnames as well as the "main" name.
> If we propagate the name we'll step back into the rtnetlink naming
> mess :(

OK, I see. I didn't realize this was a thing. I suppose what you are saying
is that we wouldn't want to expose names at all and stick with ifindexes
only, is that 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: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 0/2] Expose netdev name in netdev netlink APIs

[Jakub Kicinski]

On Wed, 21 Feb 2024 11:21:23 -0800 Joe Damato wrote:
> > For context, the reason why I left the names out is that they can change
> > at any moment, but primarily because there are also altnames now:
> > 
> > 2: eth0:
> > [...]
> >     altname enp2s0np0
> > 
> > Most of the APIs try to accept altnames as well as the "main" name.
> > If we propagate the name we'll step back into the rtnetlink naming
> > mess :(  
> 
> OK, I see. I didn't realize this was a thing. I suppose what you are saying
> is that we wouldn't want to expose names at all and stick with ifindexes
> only, is that right?

If you think it's a major usability improvement I can be convinced,
but yes, leaving the names out initially was indeed intentional.

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

[Joe Damato]

On Wed, Feb 21, 2024 at 11:26:44AM -0800, Jakub Kicinski wrote:
> On Wed, 21 Feb 2024 11:21:23 -0800 Joe Damato wrote:
> > > For context, the reason why I left the names out is that they can change
> > > at any moment, but primarily because there are also altnames now:
> > > 
> > > 2: eth0:
> > > [...]
> > >     altname enp2s0np0
> > > 
> > > Most of the APIs try to accept altnames as well as the "main" name.
> > > If we propagate the name we'll step back into the rtnetlink naming
> > > mess :(  
> > 
> > OK, I see. I didn't realize this was a thing. I suppose what you are saying
> > is that we wouldn't want to expose names at all and stick with ifindexes
> > only, is that right?
> 
> If you think it's a major usability improvement I can be convinced,
> but yes, leaving the names out initially was indeed intentional.

Well... it is useful to me, but I think I'm only one user and the side
effects of adding this might have painful results in the future so after
your comment I think it might be best left out.

Sorry for the noise.

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