Re: [PATCH] kni: fix ioctl signature

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/20/2021 1:14 PM, Markus Theil wrote:
> From: Markus Theil <markus.theil@secunet.com>
> 
> Fix kni's ioctl signature to correctly match the kernel's
> structs. This shaves off the (void*) casts and uses struct file*
> instead of struct inode*. With the correct signature, control flow
> integrity checkers are no longer confused at this point.
> 

+1 to update the signature, the code is like this from the initial release,
not sure why.

My concern was if this cause build issues with old kernel versions, but as
far as I can see the signature is like this at least since 2005, so that
should be OK.

A few minor comments below.

And overall for the patch, I suggest postponing this to next release, since
we are just about the release v21.11.
Although change looks safe and trivial, still I think it doesn't worth the risk,
taking account that current code is at it is for years now, it can wait a
little more.


> Signed-off-by: Markus Theil <markus.theil@secunet.com>
> Tested-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> ---
>   kernel/linux/kni/kni_misc.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index f4944e1ddf..d4e00abc40 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -245,7 +245,7 @@ kni_check_param(struct kni_dev *kni, struct rte_kni_device_info *dev)
>   	return 0;
>   }
>   
> -static int
> +static long
>   kni_run_thread(struct kni_net *knet, struct kni_dev *kni, uint8_t force_bind)
>   {
>   	/**
> @@ -286,12 +286,12 @@ kni_run_thread(struct kni_net *knet, struct kni_dev *kni, uint8_t force_bind)
>   	return 0;
>   }
>   
> -static int
> +static long
>   kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>   		unsigned long ioctl_param)
>   {
>   	struct kni_net *knet = net_generic(net, kni_net_id);
> -	int ret;
> +	long ret;
>   	struct rte_kni_device_info dev_info;
>   	struct net_device *net_dev = NULL;
>   	struct kni_dev *kni, *dev, *n;
> @@ -416,7 +416,7 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>   
>   	ret = register_netdev(net_dev);
>   	if (ret) {
> -		pr_err("error %i registering device \"%s\"\n",
> +		pr_err("error %li registering device \"%s\"\n",
>   					ret, dev_info.name);
>   		kni->net_dev = NULL;
>   		kni_dev_remove(kni);> @@ -437,12 +437,12 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>   	return 0;
>   }
>   
> -static int
> +static long
>   kni_ioctl_release(struct net *net, uint32_t ioctl_num,
>   		unsigned long ioctl_param)
>   {
>   	struct kni_net *knet = net_generic(net, kni_net_id);
> -	int ret = -EINVAL;
> +	long ret = -EINVAL;
>   	struct kni_dev *dev, *n;
>   	struct rte_kni_device_info dev_info;
>   

'kni_run_thread()', 'kni_ioctl_create()' & 'kni_ioctl_release()' are all
static functions, I think it is OK to keep their signature.
At works their return value assigned to a 'long' in 'kni_ioctl()', which
is safe.

> @@ -478,8 +478,8 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
>   	return ret;
>   }
>   
> -static int
> -kni_ioctl(struct inode *inode, uint32_t ioctl_num, unsigned long ioctl_param)
> +static long
> +kni_ioctl(struct file *file, uint32_t ioctl_num, unsigned long ioctl_param)
>   {
>   	int ret = -EINVAL;

What do you think about changing 'ret' variable type to 'long'?
It will be implicitly cast to 'long' anyway during return, but keeping it
same with return type looks more proper.

>   	struct net *net = current->nsproxy->net_ns;
> @@ -507,8 +507,8 @@ kni_ioctl(struct inode *inode, uint32_t ioctl_num, unsigned long ioctl_param)
>   	return ret;
>   }
>   
> -static int
> -kni_compat_ioctl(struct inode *inode, uint32_t ioctl_num,
> +static long
> +kni_compat_ioctl(struct file *file, uint32_t ioctl_num,
>   		unsigned long ioctl_param)
>   {
>   	/* 32 bits app on 64 bits OS to be supported later */
> @@ -521,8 +521,8 @@ static const struct file_operations kni_fops = {
>   	.owner = THIS_MODULE,
>   	.open = kni_open,
>   	.release = kni_release,
> -	.unlocked_ioctl = (void *)kni_ioctl,
> -	.compat_ioctl = (void *)kni_compat_ioctl,
> +	.unlocked_ioctl = kni_ioctl,
> +	.compat_ioctl = kni_compat_ioctl,
>   };
>   
>   static struct miscdevice kni_misc = {
>