RE: [PATCH net-next v3] nfc: Allow to create multiple virtual nci devices

[Bongsu Jeon <bongsu.jeon@samsung.com>]

On Sat, Nov 5, 2022 at 2:04 AM Dmitry Vyukov<dvyukov@google.com> wrote:
> The current virtual nci driver is great for testing and fuzzing.
> But it allows to create at most one "global" device which does not allow
> to run parallel tests and harms fuzzing isolation and reproducibility.
> Restructure the driver to allow creation of multiple independent devices.
> This should be backwards compatible for existing tests.

I totally agree with you for parallel tests and good design.
Thanks for good idea.
But please check the abnormal situation.
for example virtual device app is closed(virtual_ncidev_close) first and then
virtual nci driver from nci app tries to call virtual_nci_send or virtual_nci_close.
(there would be problem in virtual_nci_send because of already destroyed mutex)
Before this patch, this driver used virtual_ncidev_mode state and nci_mutex that isn't destroyed.

> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Bongsu Jeon <bongsu.jeon@samsung.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: netdev@vger.kernel.org
> 
> ---
> Changes in v3:
>  - free vdev in virtual_ncidev_close()
> 
> Changes in v2:
>  - check return value of skb_clone()
>  - rebase onto currnet net-next
> ---
>  drivers/nfc/virtual_ncidev.c | 147 +++++++++++++++++------------------
>  1 file changed, 71 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/nfc/virtual_ncidev.c b/drivers/nfc/virtual_ncidev.c
> index 85c06dbb2c449..bb76c7c7cc822 100644
> --- a/drivers/nfc/virtual_ncidev.c
> +++ b/drivers/nfc/virtual_ncidev.c
> @@ -13,12 +13,6 @@
>  
>  static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb)
>  {
> -	mutex_lock(&nci_mutex);
> -	if (state != virtual_ncidev_enabled) {
> -		mutex_unlock(&nci_mutex);
> +	struct virtual_nci_dev *vdev = nci_get_drvdata(ndev);
> +
> +	mutex_lock(&vdev->mtx);
  
  I think this vdev and vdev->mtx are already destroyed so that it would be problem.

> +	if (vdev->send_buff) {
> +		mutex_unlock(&vdev->mtx);
>  		kfree_skb(skb);
> -		return 0;
> +		return -1;
>  	}
> 
> 	
>  static int virtual_ncidev_close(struct inode *inode, struct file *file)
>  {
> -	mutex_lock(&nci_mutex);
> -
> -	if (state == virtual_ncidev_enabled) {
> -		state = virtual_ncidev_disabling;
> -		mutex_unlock(&nci_mutex);
> +	struct virtual_nci_dev *vdev = file->private_data;
>  
> -		nci_unregister_device(ndev);
> -		nci_free_device(ndev);
> -
> -		mutex_lock(&nci_mutex);
> -	}
> -
> -	state = virtual_ncidev_disabled;
> -	mutex_unlock(&nci_mutex);
> +	nci_unregister_device(vdev->ndev);
> +	nci_free_device(vdev->ndev);
> +	mutex_destroy(&vdev->mtx);
> +	kfree(vdev);
>  
>  	return 0;
>  }