Received-SPF: pass (google.com: domain of linux-kernel+bounces-75349-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <8d34d621-421b-40bd-98ab-c93783408d74@intel.com>
Date: Wed, 21 Feb 2024 11:12:47 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 1/2] netdev-genl: Add ifname for queue and NAPI
 APIs
To: Joe Damato <jdamato@fastly.com>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Jesper Dangaard Brouer <hawk@kernel.org>, "Stanislav
 Fomichev" <sdf@google.com>, Larysa Zaremba <larysa.zaremba@intel.com>,
	"Lorenzo Bianconi" <lorenzo@kernel.org>, Tariq Toukan <tariqt@nvidia.com>,
	"Sridhar Samudrala" <sridhar.samudrala@intel.com>, Alexei Starovoitov
	<ast@kernel.org>, Maciej Fijalkowski <maciej.fijalkowski@intel.com>
References: <1708531057-67392-1-git-send-email-jdamato@fastly.com>
 <1708531057-67392-2-git-send-email-jdamato@fastly.com>
Content-Language: en-US
From: "Nambiar, Amritha" <amritha.nambiar@intel.com>
In-Reply-To: <1708531057-67392-2-git-send-email-jdamato@fastly.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cm5jV1JxeXo4WE8yK0V4NUNSaDYvdjZCSExiV08rNnl1eXBxbjNzUXRVUVIr?=
 =?utf-8?B?SDI1OG1tRktncERMNWd5SC9hSzNqdHhSbXZHSFg3eHIxdVhheGNaZk45aXVk?=
 =?utf-8?B?TTBSdVk0bFQraHEvdlhyejZLMXZqcnhIOTk4Q3NpbjNvOW9ZcnE1NDdqb2lE?=
 =?utf-8?B?a1pkak0xcVRSTFFIM2RZaFhPRS9ld0l6eSt6SXZkdWtxVDJGZ1lBd3h5Vm9j?=
 =?utf-8?B?b0JvT1FIcFROSU5MLys1RkNBeERrVTBkaisxa1lCNXdRZi9vb0dYeXdiYWJO?=
 =?utf-8?B?RFYvMHY4cUgzK0Y3Tmc2SEJkNFBPMVZBUzlFM01nWUZtLzVSdHUvOWl1d1ZV?=
 =?utf-8?B?cmdsK0p3Qk1pM0pzRjN6ZVBIb0lmcmdzbEV2WkJ0MlB1aWNrNVhDL3dMSHE5?=
 =?utf-8?B?NzFnUWsxcUhKa09SMDFTNWx5WUZkN3lGQnAyeU1PQXhobjBOV1JoTnQ5R081?=
 =?utf-8?B?NmRRa1cyTkp3MW05RUhBd29xVE82SVNKMkc1MjBtUnk1ajZmNjYrUGlTUzA4?=
 =?utf-8?B?RDNLOGNzSytNalJnUFFITWRBMUVnUEd2UVVYbjhHL2QyNzdEbHZwTHg3Vlpt?=
 =?utf-8?B?TGswNmlwZzdJbGd3ZnlpeWxmZHY2UzArY1NMdzZ6eDN6dGZOWHJld3dNTGl5?=
 =?utf-8?B?QmVqVVFyeDZzVFYvYUNVVUd3RGpUS285cFhFVDVmUUxXRVdZTUJsYUF5cmFr?=
 =?utf-8?B?ekpJcm95ZXpoUTNhL2ZhQUY0bG1nT1BXV0hTY0hQUjNVNjF3Y3Y5dm1TZ2U0?=
 =?utf-8?B?NkhIQjhxa2JKNElKWDNvNWJSUDJNRDNOUkRNYnpZdDlmYmZnaG9TZkFBV2RJ?=
 =?utf-8?B?cDdHZDA4blp2QU5uOUNHL0NZR0dQcFlsUXNjOWhSbGM5M0NMUWJLSDN1Y2Vx?=
 =?utf-8?B?bXFoY0prR0IxeHcxWDVRbFBENUF6ZmZKSXE4dmhRLzhVOWNNMGZ0WUU2ckhp?=
 =?utf-8?B?WWs1bzg0VXc4L3N0a3lZejNzUi9KYW54RUMzTkVGYXFUWEF4NW83TW9OYVF4?=
 =?utf-8?B?dlh0MVZvanFrd2VXV3lSR0lRZEJEenR4L2x5Ymk1MGN1SG1IUkI4aklhdWhp?=
 =?utf-8?B?elZFSWFBWnYvSFFBZk9JMlBEVWlnRWdyOHVuZ291aFNzMDRwbU9sT3NDbmlp?=
 =?utf-8?B?S2JSVDI1cGdQdmdTSlVwT1RaOGpRWWxudk5rd2NLQ21JSVZUdFMrckFjOGRO?=
 =?utf-8?B?d1VBcUVBQVBzTGxUUkUxQnB5c3ZXb25hU1ZNOEd6ZGhpVFo2bERkQXFjTmFG?=
 =?utf-8?B?S0xkK2x6eG4wTFVna0ZvVUVEazAxSGJFU0IwUlRybXRrdkIwUUtmcGtLZkd4?=
 =?utf-8?B?TXArelpuTllObEpzL2dpU01McXFTb0ZSWEhLd084L3Q5TkZ3cFAwaXR5dUhN?=
 =?utf-8?B?TTI5RTZ1UWRVeUZtN3kveExzUlFFS1N3dmRXYVNHZXhKaXFEYy9CcTZ3QmdM?=
 =?utf-8?B?dnl3OW14cGc2TWZudmJOQ2pQK09SNzg3YnN6ZVpKZHlxSFowRHM0KzNzMXZo?=
 =?utf-8?B?dHRNS0lpUEVMOHluYmxHNEVjVG1PR0V1MUhsRElGNjlMWHhpNUNRSDB1YzR1?=
 =?utf-8?B?U3lqRFpSRjZWb1R2UnZhUlN5QXhTQlRWMHRjMXAzMlhCZG1QSWlUY0YxMnN5?=
 =?utf-8?B?TExyOWhUSENQV0JlbHppUXhWbmVickZhbkNQeDBzbEsrOWRFOUMrMGhucG1x?=
 =?utf-8?B?SlBmcnp2bkZhczI0eUQ3bW9BUFFML29XSkhIc3B5b1RkSXhKcVNJOGxpQ25P?=
 =?utf-8?B?SDdLdlZNZWwzZ2xiYXkvbC92OTlvZzA3czJINWdxcjI0TXhPSFBBSlZjTXZr?=
 =?utf-8?B?MzdYMWxRekVXcXBYdUNVUzlubERNcGdlODRZV29aTTc0VkFFcVh4NzFMbWFP?=
 =?utf-8?B?K3Q5UnBHeUFvMU0vMVhxdzAyZTl5cnU0Tmw3VFJKTWhJbk9QRTN3S1JpM295?=
 =?utf-8?B?V09EOWwrbHdiUEtvUXNwYWVuT2hpcHhjWTVHTXVOUzF1bForR2xxK3AvZUFX?=
 =?utf-8?B?eW9ucXNrUXFpY0pBUndGMWVvWHRHalhSVG9lVTMrYjVDUGZsUW5tUmxleURG?=
 =?utf-8?B?bEJIaHFzaEp3MnN1a0xMRG54RWo0bUcyQmZJTGJLU2R2QWFhVDU4aXI4VUho?=
 =?utf-8?B?V1FSRWhWWGxnQTFNRTJtVHhCN2dZM0xwRkFjRGs3M1p6d0FaMmZrWUh0d0lt?=
 =?utf-8?B?Y3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b18379b-342a-4db1-5d5f-08dc33111955
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB7869.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2024 19:12:51.4725
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9Aw0g+s09lS3kAFgl/dUh3AhZxQPy7Qh7gfMrRSULYKBs3kgNqETdwCnbAuPDWSJqqpvLNxULsQzjYkzPZzymTStZXR6llYW6GpbMHb8rJA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5394
X-OriginatorOrg: intel.com

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();
>