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

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

[Dmitry Vyukov]

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.

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 @@
 #include <linux/wait.h>
 #include <net/nfc/nci_core.h>
 
-enum virtual_ncidev_mode {
-	virtual_ncidev_enabled,
-	virtual_ncidev_disabled,
-	virtual_ncidev_disabling,
-};
-
 #define IOCTL_GET_NCIDEV_IDX    0
 #define VIRTUAL_NFC_PROTOCOLS	(NFC_PROTO_JEWEL_MASK | \
 				 NFC_PROTO_MIFARE_MASK | \
@@ -27,12 +21,12 @@ enum virtual_ncidev_mode {
 				 NFC_PROTO_ISO14443_B_MASK | \
 				 NFC_PROTO_ISO15693_MASK)
 
-static enum virtual_ncidev_mode state;
-static DECLARE_WAIT_QUEUE_HEAD(wq);
-static struct miscdevice miscdev;
-static struct sk_buff *send_buff;
-static struct nci_dev *ndev;
-static DEFINE_MUTEX(nci_mutex);
+struct virtual_nci_dev {
+	struct nci_dev *ndev;
+	struct mutex mtx;
+	struct sk_buff *send_buff;
+	struct wait_queue_head wq;
+};
 
 static int virtual_nci_open(struct nci_dev *ndev)
 {
@@ -41,31 +35,34 @@ static int virtual_nci_open(struct nci_dev *ndev)
 
 static int virtual_nci_close(struct nci_dev *ndev)
 {
-	mutex_lock(&nci_mutex);
-	kfree_skb(send_buff);
-	send_buff = NULL;
-	mutex_unlock(&nci_mutex);
+	struct virtual_nci_dev *vdev = nci_get_drvdata(ndev);
+
+	mutex_lock(&vdev->mtx);
+	kfree_skb(vdev->send_buff);
+	vdev->send_buff = NULL;
+	mutex_unlock(&vdev->mtx);
 
 	return 0;
 }
 
 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);
+	if (vdev->send_buff) {
+		mutex_unlock(&vdev->mtx);
 		kfree_skb(skb);
-		return 0;
+		return -1;
 	}
-
-	if (send_buff) {
-		mutex_unlock(&nci_mutex);
+	vdev->send_buff = skb_copy(skb, GFP_KERNEL);
+	if (!vdev->send_buff) {
+		mutex_unlock(&vdev->mtx);
 		kfree_skb(skb);
 		return -1;
 	}
-	send_buff = skb_copy(skb, GFP_KERNEL);
-	mutex_unlock(&nci_mutex);
-	wake_up_interruptible(&wq);
+	mutex_unlock(&vdev->mtx);
+	wake_up_interruptible(&vdev->wq);
 	consume_skb(skb);
 
 	return 0;
@@ -80,29 +77,30 @@ static const struct nci_ops virtual_nci_ops = {
 static ssize_t virtual_ncidev_read(struct file *file, char __user *buf,
 				   size_t count, loff_t *ppos)
 {
+	struct virtual_nci_dev *vdev = file->private_data;
 	size_t actual_len;
 
-	mutex_lock(&nci_mutex);
-	while (!send_buff) {
-		mutex_unlock(&nci_mutex);
-		if (wait_event_interruptible(wq, send_buff))
+	mutex_lock(&vdev->mtx);
+	while (!vdev->send_buff) {
+		mutex_unlock(&vdev->mtx);
+		if (wait_event_interruptible(vdev->wq, vdev->send_buff))
 			return -EFAULT;
-		mutex_lock(&nci_mutex);
+		mutex_lock(&vdev->mtx);
 	}
 
-	actual_len = min_t(size_t, count, send_buff->len);
+	actual_len = min_t(size_t, count, vdev->send_buff->len);
 
-	if (copy_to_user(buf, send_buff->data, actual_len)) {
-		mutex_unlock(&nci_mutex);
+	if (copy_to_user(buf, vdev->send_buff->data, actual_len)) {
+		mutex_unlock(&vdev->mtx);
 		return -EFAULT;
 	}
 
-	skb_pull(send_buff, actual_len);
-	if (send_buff->len == 0) {
-		consume_skb(send_buff);
-		send_buff = NULL;
+	skb_pull(vdev->send_buff, actual_len);
+	if (vdev->send_buff->len == 0) {
+		consume_skb(vdev->send_buff);
+		vdev->send_buff = NULL;
 	}
-	mutex_unlock(&nci_mutex);
+	mutex_unlock(&vdev->mtx);
 
 	return actual_len;
 }
@@ -111,6 +109,7 @@ static ssize_t virtual_ncidev_write(struct file *file,
 				    const char __user *buf,
 				    size_t count, loff_t *ppos)
 {
+	struct virtual_nci_dev *vdev = file->private_data;
 	struct sk_buff *skb;
 
 	skb = alloc_skb(count, GFP_KERNEL);
@@ -122,63 +121,58 @@ static ssize_t virtual_ncidev_write(struct file *file,
 		return -EFAULT;
 	}
 
-	nci_recv_frame(ndev, skb);
+	nci_recv_frame(vdev->ndev, skb);
 	return count;
 }
 
 static int virtual_ncidev_open(struct inode *inode, struct file *file)
 {
 	int ret = 0;
+	struct virtual_nci_dev *vdev;
 
-	mutex_lock(&nci_mutex);
-	if (state != virtual_ncidev_disabled) {
-		mutex_unlock(&nci_mutex);
-		return -EBUSY;
-	}
-
-	ndev = nci_allocate_device(&virtual_nci_ops, VIRTUAL_NFC_PROTOCOLS,
-				   0, 0);
-	if (!ndev) {
-		mutex_unlock(&nci_mutex);
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+	vdev->ndev = nci_allocate_device(&virtual_nci_ops,
+		VIRTUAL_NFC_PROTOCOLS, 0, 0);
+	if (!vdev->ndev) {
+		kfree(vdev);
 		return -ENOMEM;
 	}
 
-	ret = nci_register_device(ndev);
+	mutex_init(&vdev->mtx);
+	init_waitqueue_head(&vdev->wq);
+	file->private_data = vdev;
+	nci_set_drvdata(vdev->ndev, vdev);
+
+	ret = nci_register_device(vdev->ndev);
 	if (ret < 0) {
-		nci_free_device(ndev);
-		mutex_unlock(&nci_mutex);
+		nci_free_device(vdev->ndev);
+		mutex_destroy(&vdev->mtx);
+		kfree(vdev);
 		return ret;
 	}
-	state = virtual_ncidev_enabled;
-	mutex_unlock(&nci_mutex);
 
 	return 0;
 }
 
 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;
 }
 
-static long virtual_ncidev_ioctl(struct file *flip, unsigned int cmd,
+static long virtual_ncidev_ioctl(struct file *file, unsigned int cmd,
 				 unsigned long arg)
 {
-	const struct nfc_dev *nfc_dev = ndev->nfc_dev;
+	struct virtual_nci_dev *vdev = file->private_data;
+	const struct nfc_dev *nfc_dev = vdev->ndev->nfc_dev;
 	void __user *p = (void __user *)arg;
 
 	if (cmd != IOCTL_GET_NCIDEV_IDX)
@@ -199,14 +193,15 @@ static const struct file_operations virtual_ncidev_fops = {
 	.unlocked_ioctl = virtual_ncidev_ioctl
 };
 
+static struct miscdevice miscdev = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "virtual_nci",
+	.fops = &virtual_ncidev_fops,
+	.mode = 0600,
+};
+
 static int __init virtual_ncidev_init(void)
 {
-	state = virtual_ncidev_disabled;
-	miscdev.minor = MISC_DYNAMIC_MINOR;
-	miscdev.name = "virtual_nci";
-	miscdev.fops = &virtual_ncidev_fops;
-	miscdev.mode = 0600;
-
 	return misc_register(&miscdev);
 }
 

base-commit: d9095f92950bd16745b9ec24ebebc12d14b3a3e8
-- 
2.38.1.431.g37b22c650d-goog

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

[Bongsu Jeon]

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;
>  }

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

[Dmitry Vyukov]

On Sun, 6 Nov 2022 at 18:46, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
>
> 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.

I assumed nci core must stop calling into a driver at some point
during the driver destruction. And I assumed that point is return from
nci_unregister_device(). Basically when nci_unregister_device()
returns, no new calls into the driver must be made. Calling into a
driver after nci_unregister_device() looks like a bug in nci core.

If this is not true, how do real drivers handle this? They don't use
global vars. So they should either have the same use-after-free bugs
you described, or they handle shutdown differently. We just need to do
the same thing that real drivers do.

As far as I see they are doing the same what I did in this patch:
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354

They call nci_unregister_device() and then free all resources:
https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186

What am I missing here?


> > 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;
> >  }

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

[Bongsu Jeon]

On Tue, Nov 8, 2022 at 3:38 AM Dmitry Vyukov<dvyukov@google.com> wrote:
> On Sun, 6 Nov 2022 at 18:46, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> >
> > 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.
> 
> I assumed nci core must stop calling into a driver at some point
> during the driver destruction. And I assumed that point is return from
> nci_unregister_device(). Basically when nci_unregister_device()
> returns, no new calls into the driver must be made. Calling into a
> driver after nci_unregister_device() looks like a bug in nci core.
> 
> If this is not true, how do real drivers handle this? They don't use
> global vars. So they should either have the same use-after-free bugs
> you described, or they handle shutdown differently. We just need to do
> the same thing that real drivers do.
> 
> As far as I see they are doing the same what I did in this patch:
> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> 
> They call nci_unregister_device() and then free all resources:
> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> 
> What am I missing here?

I'm not sure but I think they are little different.
nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
But virtual_ncidev just uses file operation(close function) not related to driver.
so Nci simulation App can call close function at any time.
If Scheduler interrupts the nci core code right after calling virtual_nci_send and then 
other process or thread calls virtual_nci_dev's close function,
we need to handle this problem in virtual nci driver.

> > > 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;
> > >  }

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

[Dmitry Vyukov]

On Mon, 7 Nov 2022 at 16:43, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
>
> On Tue, Nov 8, 2022 at 3:38 AM Dmitry Vyukov<dvyukov@google.com> wrote:
> > On Sun, 6 Nov 2022 at 18:46, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> > >
> > > 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.
> >
> > I assumed nci core must stop calling into a driver at some point
> > during the driver destruction. And I assumed that point is return from
> > nci_unregister_device(). Basically when nci_unregister_device()
> > returns, no new calls into the driver must be made. Calling into a
> > driver after nci_unregister_device() looks like a bug in nci core.
> >
> > If this is not true, how do real drivers handle this? They don't use
> > global vars. So they should either have the same use-after-free bugs
> > you described, or they handle shutdown differently. We just need to do
> > the same thing that real drivers do.
> >
> > As far as I see they are doing the same what I did in this patch:
> > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> >
> > They call nci_unregister_device() and then free all resources:
> > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> >
> > What am I missing here?
>
> I'm not sure but I think they are little different.
> nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> But virtual_ncidev just uses file operation(close function) not related to driver.
> so Nci simulation App can call close function at any time.
> If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> other process or thread calls virtual_nci_dev's close function,
> we need to handle this problem in virtual nci driver.

Won't the same issue happen if nci send callback is concurrent with
USB/I2C driver disconnect?

I mean something internal to the USB subsystem cannot affect what nci
subsystem is doing, unless the USB driver calls into nci and somehow
notifies it that it's about to destroy the driver.

Is there anything USB/I2C drivers are doing besides calling
nci_unregister_device() to ensure that there are no pending nci send
calls? If yes, then we should do the same in the virtual driver. If
not, then all other drivers are the subject to the same use-after-free
bug.

But I assumed that nci_unregister_device() ensures that there are no
in-flight send calls and no future send calls will be issued after the
function returns.

> > > > 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;
> > > >  }
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20221108004316epcms2p63ff537496ef759cb0c734068bd58855c%40epcms2p6.

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

[Bongsu Jeon]

On Wed, Nov 9, 2022 at 7:52 AM Dmitry Vyukov<dvyukov@google.com> wrote:
> On Mon, 7 Nov 2022 at 16:43, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> >
> > On Tue, Nov 8, 2022 at 3:38 AM Dmitry Vyukov<dvyukov@google.com> wrote:
> > > On Sun, 6 Nov 2022 at 18:46, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> > > >
> > > > 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.
> > >
> > > I assumed nci core must stop calling into a driver at some point
> > > during the driver destruction. And I assumed that point is return from
> > > nci_unregister_device(). Basically when nci_unregister_device()
> > > returns, no new calls into the driver must be made. Calling into a
> > > driver after nci_unregister_device() looks like a bug in nci core.
> > >
> > > If this is not true, how do real drivers handle this? They don't use
> > > global vars. So they should either have the same use-after-free bugs
> > > you described, or they handle shutdown differently. We just need to do
> > > the same thing that real drivers do.
> > >
> > > As far as I see they are doing the same what I did in this patch:
> > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> > >
> > > They call nci_unregister_device() and then free all resources:
> > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> > >
> > > What am I missing here?
> >
> > I'm not sure but I think they are little different.
> > nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> > But virtual_ncidev just uses file operation(close function) not related to driver.
> > so Nci simulation App can call close function at any time.
> > If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> > other process or thread calls virtual_nci_dev's close function,
> > we need to handle this problem in virtual nci driver.
> 
> Won't the same issue happen if nci send callback is concurrent with
> USB/I2C driver disconnect?
> 
> I mean something internal to the USB subsystem cannot affect what nci
> subsystem is doing, unless the USB driver calls into nci and somehow
> notifies it that it's about to destroy the driver.
> 
> Is there anything USB/I2C drivers are doing besides calling
> nci_unregister_device() to ensure that there are no pending nci send
> calls? If yes, then we should do the same in the virtual driver. If
> not, then all other drivers are the subject to the same use-after-free
> bug.
> 
> But I assumed that nci_unregister_device() ensures that there are no
> in-flight send calls and no future send calls will be issued after the
> function returns.

Ok, I understand your mention. you mean that nci_unregister_device should prevent 
the issue using dev lock or other way. right?
It would be better to handle the issue in nci core if there is.

> 
> > > > > 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;
> > > > >  }
> 

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

[Dmitry Vyukov]

On Tue, 8 Nov 2022 at 16:35, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> > > > > 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.
> > > >
> > > > I assumed nci core must stop calling into a driver at some point
> > > > during the driver destruction. And I assumed that point is return from
> > > > nci_unregister_device(). Basically when nci_unregister_device()
> > > > returns, no new calls into the driver must be made. Calling into a
> > > > driver after nci_unregister_device() looks like a bug in nci core.
> > > >
> > > > If this is not true, how do real drivers handle this? They don't use
> > > > global vars. So they should either have the same use-after-free bugs
> > > > you described, or they handle shutdown differently. We just need to do
> > > > the same thing that real drivers do.
> > > >
> > > > As far as I see they are doing the same what I did in this patch:
> > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> > > >
> > > > They call nci_unregister_device() and then free all resources:
> > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> > > >
> > > > What am I missing here?
> > >
> > > I'm not sure but I think they are little different.
> > > nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> > > But virtual_ncidev just uses file operation(close function) not related to driver.
> > > so Nci simulation App can call close function at any time.
> > > If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> > > other process or thread calls virtual_nci_dev's close function,
> > > we need to handle this problem in virtual nci driver.
> >
> > Won't the same issue happen if nci send callback is concurrent with
> > USB/I2C driver disconnect?
> >
> > I mean something internal to the USB subsystem cannot affect what nci
> > subsystem is doing, unless the USB driver calls into nci and somehow
> > notifies it that it's about to destroy the driver.
> >
> > Is there anything USB/I2C drivers are doing besides calling
> > nci_unregister_device() to ensure that there are no pending nci send
> > calls? If yes, then we should do the same in the virtual driver. If
> > not, then all other drivers are the subject to the same use-after-free
> > bug.
> >
> > But I assumed that nci_unregister_device() ensures that there are no
> > in-flight send calls and no future send calls will be issued after the
> > function returns.
>
> Ok, I understand your mention. you mean that nci_unregister_device should prevent
> the issue using dev lock or other way. right?

Yes.

> It would be better to handle the issue in nci core if there is.

And yes.

Krzysztof, can you confirm this is the case (nci core won't call
ops->send callback after nci_unregister_device() returns)?



> > > > > > 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;
> > > > > >  }
> >

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

[Bongsu Jeon]

On Wed, Nov 9, 2022 at 9:42 AM Dmitry Vyukov<dvyukov@google.com> wrote:
> On Tue, 8 Nov 2022 at 16:35, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> > > > > > 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.
> > > > >
> > > > > I assumed nci core must stop calling into a driver at some point
> > > > > during the driver destruction. And I assumed that point is return from
> > > > > nci_unregister_device(). Basically when nci_unregister_device()
> > > > > returns, no new calls into the driver must be made. Calling into a
> > > > > driver after nci_unregister_device() looks like a bug in nci core.
> > > > >
> > > > > If this is not true, how do real drivers handle this? They don't use
> > > > > global vars. So they should either have the same use-after-free bugs
> > > > > you described, or they handle shutdown differently. We just need to do
> > > > > the same thing that real drivers do.
> > > > >
> > > > > As far as I see they are doing the same what I did in this patch:
> > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> > > > >
> > > > > They call nci_unregister_device() and then free all resources:
> > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> > > > >
> > > > > What am I missing here?
> > > >
> > > > I'm not sure but I think they are little different.
> > > > nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> > > > But virtual_ncidev just uses file operation(close function) not related to driver.
> > > > so Nci simulation App can call close function at any time.
> > > > If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> > > > other process or thread calls virtual_nci_dev's close function,
> > > > we need to handle this problem in virtual nci driver.
> > >
> > > Won't the same issue happen if nci send callback is concurrent with
> > > USB/I2C driver disconnect?
> > >
> > > I mean something internal to the USB subsystem cannot affect what nci
> > > subsystem is doing, unless the USB driver calls into nci and somehow
> > > notifies it that it's about to destroy the driver.
> > >
> > > Is there anything USB/I2C drivers are doing besides calling
> > > nci_unregister_device() to ensure that there are no pending nci send
> > > calls? If yes, then we should do the same in the virtual driver. If
> > > not, then all other drivers are the subject to the same use-after-free
> > > bug.
> > >
> > > But I assumed that nci_unregister_device() ensures that there are no
> > > in-flight send calls and no future send calls will be issued after the
> > > function returns.
> >
> > Ok, I understand your mention. you mean that nci_unregister_device should prevent
> > the issue using dev lock or other way. right?
> 
> Yes.
> 
> > It would be better to handle the issue in nci core if there is.
> 
> And yes.
> 
> Krzysztof, can you confirm this is the case (nci core won't call
> ops->send callback after nci_unregister_device() returns)?

I think we can add this to selftest to verify nci core.

> 
> 
> > > > > > > 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;
> > > > > > >  }
> > >

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

[Dmitry Vyukov]

On Mon, 14 Nov 2022 at 00:32, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> > > > > > > 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.
> > > > > >
> > > > > > I assumed nci core must stop calling into a driver at some point
> > > > > > during the driver destruction. And I assumed that point is return from
> > > > > > nci_unregister_device(). Basically when nci_unregister_device()
> > > > > > returns, no new calls into the driver must be made. Calling into a
> > > > > > driver after nci_unregister_device() looks like a bug in nci core.
> > > > > >
> > > > > > If this is not true, how do real drivers handle this? They don't use
> > > > > > global vars. So they should either have the same use-after-free bugs
> > > > > > you described, or they handle shutdown differently. We just need to do
> > > > > > the same thing that real drivers do.
> > > > > >
> > > > > > As far as I see they are doing the same what I did in this patch:
> > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> > > > > >
> > > > > > They call nci_unregister_device() and then free all resources:
> > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> > > > > >
> > > > > > What am I missing here?
> > > > >
> > > > > I'm not sure but I think they are little different.
> > > > > nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> > > > > But virtual_ncidev just uses file operation(close function) not related to driver.
> > > > > so Nci simulation App can call close function at any time.
> > > > > If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> > > > > other process or thread calls virtual_nci_dev's close function,
> > > > > we need to handle this problem in virtual nci driver.
> > > >
> > > > Won't the same issue happen if nci send callback is concurrent with
> > > > USB/I2C driver disconnect?
> > > >
> > > > I mean something internal to the USB subsystem cannot affect what nci
> > > > subsystem is doing, unless the USB driver calls into nci and somehow
> > > > notifies it that it's about to destroy the driver.
> > > >
> > > > Is there anything USB/I2C drivers are doing besides calling
> > > > nci_unregister_device() to ensure that there are no pending nci send
> > > > calls? If yes, then we should do the same in the virtual driver. If
> > > > not, then all other drivers are the subject to the same use-after-free
> > > > bug.
> > > >
> > > > But I assumed that nci_unregister_device() ensures that there are no
> > > > in-flight send calls and no future send calls will be issued after the
> > > > function returns.
> > >
> > > Ok, I understand your mention. you mean that nci_unregister_device should prevent
> > > the issue using dev lock or other way. right?
> >
> > Yes.
> >
> > > It would be better to handle the issue in nci core if there is.
> >
> > And yes.
> >
> > Krzysztof, can you confirm this is the case (nci core won't call
> > ops->send callback after nci_unregister_device() returns)?
>
> I think we can add this to selftest to verify nci core.

I am not sure how the test for that particular scenario should look
like. It's only possible with concurrent syscalls, right? After
nci_unregister_device() returns and the virtual device fd is closed,
it's not possible to trigger the send callback, right?


> > > > > > > > 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;
> > > > > > > >  }
> > > >

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

[Bongsu Jeon]

On Mon, Nov 14, 2022 at 6:54 PM Dmitry Vyukov<dvyukov@google.com> wrote:
> On Mon, 14 Nov 2022 at 00:32, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
> > > > > > > > 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.
> > > > > > >
> > > > > > > I assumed nci core must stop calling into a driver at some point
> > > > > > > during the driver destruction. And I assumed that point is return from
> > > > > > > nci_unregister_device(). Basically when nci_unregister_device()
> > > > > > > returns, no new calls into the driver must be made. Calling into a
> > > > > > > driver after nci_unregister_device() looks like a bug in nci core.
> > > > > > >
> > > > > > > If this is not true, how do real drivers handle this? They don't use
> > > > > > > global vars. So they should either have the same use-after-free bugs
> > > > > > > you described, or they handle shutdown differently. We just need to do
> > > > > > > the same thing that real drivers do.
> > > > > > >
> > > > > > > As far as I see they are doing the same what I did in this patch:
> > > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> > > > > > >
> > > > > > > They call nci_unregister_device() and then free all resources:
> > > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> > > > > > >
> > > > > > > What am I missing here?
> > > > > >
> > > > > > I'm not sure but I think they are little different.
> > > > > > nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> > > > > > But virtual_ncidev just uses file operation(close function) not related to driver.
> > > > > > so Nci simulation App can call close function at any time.
> > > > > > If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> > > > > > other process or thread calls virtual_nci_dev's close function,
> > > > > > we need to handle this problem in virtual nci driver.
> > > > >
> > > > > Won't the same issue happen if nci send callback is concurrent with
> > > > > USB/I2C driver disconnect?
> > > > >
> > > > > I mean something internal to the USB subsystem cannot affect what nci
> > > > > subsystem is doing, unless the USB driver calls into nci and somehow
> > > > > notifies it that it's about to destroy the driver.
> > > > >
> > > > > Is there anything USB/I2C drivers are doing besides calling
> > > > > nci_unregister_device() to ensure that there are no pending nci send
> > > > > calls? If yes, then we should do the same in the virtual driver. If
> > > > > not, then all other drivers are the subject to the same use-after-free
> > > > > bug.
> > > > >
> > > > > But I assumed that nci_unregister_device() ensures that there are no
> > > > > in-flight send calls and no future send calls will be issued after the
> > > > > function returns.
> > > >
> > > > Ok, I understand your mention. you mean that nci_unregister_device should prevent
> > > > the issue using dev lock or other way. right?
> > >
> > > Yes.
> > >
> > > > It would be better to handle the issue in nci core if there is.
> > >
> > > And yes.
> > >
> > > Krzysztof, can you confirm this is the case (nci core won't call
> > > ops->send callback after nci_unregister_device() returns)?
> >
> > I think we can add this to selftest to verify nci core.
> 
> I am not sure how the test for that particular scenario should look
> like. It's only possible with concurrent syscalls, right? After
> nci_unregister_device() returns and the virtual device fd is closed,
> it's not possible to trigger the send callback, right?
> 

As you think, we can't control concurrent timing so that it will be hard to
test exactly same scenario that i asked first.
I just wanted to see the simple scenario testcase with single thread.
I thought following simple sequence.

1. virtualDevFd = open() // for virtual device 
2. enable and open nci dev and connect NFC socket with virtualDevFd using NFC/NCI System Call
3. close virtualDevFd.( nci_unregister_device )
4. send test data using connected NFC socket.
=> if socket write operation failed and there were no issues in kernel, then it works properly.

> 
> > > > > > > > > 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;
> > > > > > > > >  }
> > > > >

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

[Krzysztof Kozlowski]

On 09/11/2022 01:42, Dmitry Vyukov wrote:
> On Tue, 8 Nov 2022 at 16:35, Bongsu Jeon <bongsu.jeon@samsung.com> wrote:
>>>>>> 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.
>>>>>
>>>>> I assumed nci core must stop calling into a driver at some point
>>>>> during the driver destruction. And I assumed that point is return from
>>>>> nci_unregister_device(). Basically when nci_unregister_device()
>>>>> returns, no new calls into the driver must be made. Calling into a
>>>>> driver after nci_unregister_device() looks like a bug in nci core.
>>>>>
>>>>> If this is not true, how do real drivers handle this? They don't use
>>>>> global vars. So they should either have the same use-after-free bugs
>>>>> you described, or they handle shutdown differently. We just need to do
>>>>> the same thing that real drivers do.
>>>>>
>>>>> As far as I see they are doing the same what I did in this patch:
>>>>> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
>>>>> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
>>>>>
>>>>> They call nci_unregister_device() and then free all resources:
>>>>> https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
>>>>>
>>>>> What am I missing here?
>>>>
>>>> I'm not sure but I think they are little different.
>>>> nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
>>>> But virtual_ncidev just uses file operation(close function) not related to driver.
>>>> so Nci simulation App can call close function at any time.
>>>> If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
>>>> other process or thread calls virtual_nci_dev's close function,
>>>> we need to handle this problem in virtual nci driver.
>>>
>>> Won't the same issue happen if nci send callback is concurrent with
>>> USB/I2C driver disconnect?
>>>
>>> I mean something internal to the USB subsystem cannot affect what nci
>>> subsystem is doing, unless the USB driver calls into nci and somehow
>>> notifies it that it's about to destroy the driver.
>>>
>>> Is there anything USB/I2C drivers are doing besides calling
>>> nci_unregister_device() to ensure that there are no pending nci send
>>> calls? If yes, then we should do the same in the virtual driver. If
>>> not, then all other drivers are the subject to the same use-after-free
>>> bug.
>>>
>>> But I assumed that nci_unregister_device() ensures that there are no
>>> in-flight send calls and no future send calls will be issued after the
>>> function returns.
>>
>> Ok, I understand your mention. you mean that nci_unregister_device should prevent
>> the issue using dev lock or other way. right?
> 
> Yes.
> 
>> It would be better to handle the issue in nci core if there is.
> 
> And yes.
> 
> Krzysztof, can you confirm this is the case (nci core won't call
> ops->send callback after nci_unregister_device() returns)?

You asked me like I would know. :) I took the NFC subsystem, to bring it
a bit to shape, but I did not write any of this code and I don't
actually know - until I analyze the code as we all do...

Best regards,
Krzysztof

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

[Bongsu Jeon]

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.
> 
> 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 @@
>  #include <linux/wait.h>
>  #include <net/nfc/nci_core.h>
>  
> -enum virtual_ncidev_mode {
> -	virtual_ncidev_enabled,
> -	virtual_ncidev_disabled,
> -	virtual_ncidev_disabling,
> -};
> -
>  #define IOCTL_GET_NCIDEV_IDX    0
>  #define VIRTUAL_NFC_PROTOCOLS	(NFC_PROTO_JEWEL_MASK | \
>  				 NFC_PROTO_MIFARE_MASK | \
> @@ -27,12 +21,12 @@ enum virtual_ncidev_mode {
>  				 NFC_PROTO_ISO14443_B_MASK | \
>  				 NFC_PROTO_ISO15693_MASK)
>  
> -static enum virtual_ncidev_mode state;
> -static DECLARE_WAIT_QUEUE_HEAD(wq);
> -static struct miscdevice miscdev;
> -static struct sk_buff *send_buff;
> -static struct nci_dev *ndev;
> -static DEFINE_MUTEX(nci_mutex);
> +struct virtual_nci_dev {
> +	struct nci_dev *ndev;
> +	struct mutex mtx;
> +	struct sk_buff *send_buff;
> +	struct wait_queue_head wq;
> +};
>  
>  static int virtual_nci_open(struct nci_dev *ndev)
>  {
> @@ -41,31 +35,34 @@ static int virtual_nci_open(struct nci_dev *ndev)
>  
>  static int virtual_nci_close(struct nci_dev *ndev)
>  {
> -	mutex_lock(&nci_mutex);
> -	kfree_skb(send_buff);
> -	send_buff = NULL;
> -	mutex_unlock(&nci_mutex);
> +	struct virtual_nci_dev *vdev = nci_get_drvdata(ndev);
> +
> +	mutex_lock(&vdev->mtx);
> +	kfree_skb(vdev->send_buff);
> +	vdev->send_buff = NULL;
> +	mutex_unlock(&vdev->mtx);
>  
>  	return 0;
>  }
>  
>  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);
> +	if (vdev->send_buff) {
> +		mutex_unlock(&vdev->mtx);
>  		kfree_skb(skb);
> -		return 0;
> +		return -1;
>  	}
> -
> -	if (send_buff) {
> -		mutex_unlock(&nci_mutex);
> +	vdev->send_buff = skb_copy(skb, GFP_KERNEL);
> +	if (!vdev->send_buff) {
> +		mutex_unlock(&vdev->mtx);
>  		kfree_skb(skb);
>  		return -1;
>  	}
> -	send_buff = skb_copy(skb, GFP_KERNEL);
> -	mutex_unlock(&nci_mutex);
> -	wake_up_interruptible(&wq);
> +	mutex_unlock(&vdev->mtx);
> +	wake_up_interruptible(&vdev->wq);
>  	consume_skb(skb);
>  
>  	return 0;
> @@ -80,29 +77,30 @@ static const struct nci_ops virtual_nci_ops = {
>  static ssize_t virtual_ncidev_read(struct file *file, char __user *buf,
>  				   size_t count, loff_t *ppos)
>  {
> +	struct virtual_nci_dev *vdev = file->private_data;
>  	size_t actual_len;
>  
> -	mutex_lock(&nci_mutex);
> -	while (!send_buff) {
> -		mutex_unlock(&nci_mutex);
> -		if (wait_event_interruptible(wq, send_buff))
> +	mutex_lock(&vdev->mtx);
> +	while (!vdev->send_buff) {
> +		mutex_unlock(&vdev->mtx);
> +		if (wait_event_interruptible(vdev->wq, vdev->send_buff))
>  			return -EFAULT;
> -		mutex_lock(&nci_mutex);
> +		mutex_lock(&vdev->mtx);
>  	}
>  
> -	actual_len = min_t(size_t, count, send_buff->len);
> +	actual_len = min_t(size_t, count, vdev->send_buff->len);
>  
> -	if (copy_to_user(buf, send_buff->data, actual_len)) {
> -		mutex_unlock(&nci_mutex);
> +	if (copy_to_user(buf, vdev->send_buff->data, actual_len)) {
> +		mutex_unlock(&vdev->mtx);
>  		return -EFAULT;
>  	}
>  
> -	skb_pull(send_buff, actual_len);
> -	if (send_buff->len == 0) {
> -		consume_skb(send_buff);
> -		send_buff = NULL;
> +	skb_pull(vdev->send_buff, actual_len);
> +	if (vdev->send_buff->len == 0) {
> +		consume_skb(vdev->send_buff);
> +		vdev->send_buff = NULL;
>  	}
> -	mutex_unlock(&nci_mutex);
> +	mutex_unlock(&vdev->mtx);
>  
>  	return actual_len;
>  }
> @@ -111,6 +109,7 @@ static ssize_t virtual_ncidev_write(struct file *file,
>  				    const char __user *buf,
>  				    size_t count, loff_t *ppos)
>  {
> +	struct virtual_nci_dev *vdev = file->private_data;
>  	struct sk_buff *skb;
>  
>  	skb = alloc_skb(count, GFP_KERNEL);
> @@ -122,63 +121,58 @@ static ssize_t virtual_ncidev_write(struct file *file,
>  		return -EFAULT;
>  	}
>  
> -	nci_recv_frame(ndev, skb);
> +	nci_recv_frame(vdev->ndev, skb);
>  	return count;
>  }
>  
>  static int virtual_ncidev_open(struct inode *inode, struct file *file)
>  {
>  	int ret = 0;
> +	struct virtual_nci_dev *vdev;
>  
> -	mutex_lock(&nci_mutex);
> -	if (state != virtual_ncidev_disabled) {
> -		mutex_unlock(&nci_mutex);
> -		return -EBUSY;
> -	}
> -
> -	ndev = nci_allocate_device(&virtual_nci_ops, VIRTUAL_NFC_PROTOCOLS,
> -				   0, 0);
> -	if (!ndev) {
> -		mutex_unlock(&nci_mutex);
> +	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> +	if (!vdev)
> +		return -ENOMEM;
> +	vdev->ndev = nci_allocate_device(&virtual_nci_ops,
> +		VIRTUAL_NFC_PROTOCOLS, 0, 0);
> +	if (!vdev->ndev) {
> +		kfree(vdev);
>  		return -ENOMEM;
>  	}
>  
> -	ret = nci_register_device(ndev);
> +	mutex_init(&vdev->mtx);
> +	init_waitqueue_head(&vdev->wq);
> +	file->private_data = vdev;
> +	nci_set_drvdata(vdev->ndev, vdev);
> +
> +	ret = nci_register_device(vdev->ndev);
>  	if (ret < 0) {
> -		nci_free_device(ndev);
> -		mutex_unlock(&nci_mutex);
> +		nci_free_device(vdev->ndev);
> +		mutex_destroy(&vdev->mtx);
> +		kfree(vdev);
>  		return ret;
>  	}
> -	state = virtual_ncidev_enabled;
> -	mutex_unlock(&nci_mutex);
>  
>  	return 0;
>  }
>  
>  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;
>  }
>  
> -static long virtual_ncidev_ioctl(struct file *flip, unsigned int cmd,
> +static long virtual_ncidev_ioctl(struct file *file, unsigned int cmd,
>  				 unsigned long arg)
>  {
> -	const struct nfc_dev *nfc_dev = ndev->nfc_dev;
> +	struct virtual_nci_dev *vdev = file->private_data;
> +	const struct nfc_dev *nfc_dev = vdev->ndev->nfc_dev;
>  	void __user *p = (void __user *)arg;
>  
>  	if (cmd != IOCTL_GET_NCIDEV_IDX)
> @@ -199,14 +193,15 @@ static const struct file_operations virtual_ncidev_fops = {
>  	.unlocked_ioctl = virtual_ncidev_ioctl
>  };
>  
> +static struct miscdevice miscdev = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "virtual_nci",
> +	.fops = &virtual_ncidev_fops,
> +	.mode = 0600,
> +};
> +
>  static int __init virtual_ncidev_init(void)
>  {
> -	state = virtual_ncidev_disabled;
> -	miscdev.minor = MISC_DYNAMIC_MINOR;
> -	miscdev.name = "virtual_nci";
> -	miscdev.fops = &virtual_ncidev_fops;
> -	miscdev.mode = 0600;
> -
>  	return misc_register(&miscdev);
>  }
> 

Reviewed-by: Bongsu Jeon

Thanks for good design and improvement.

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

[Jakub Kicinski]

On Mon, 14 Nov 2022 19:27:29 +0900 Bongsu Jeon wrote:
> Reviewed-by: Bongsu Jeon

Dmitry if the patch is good after all - would you mind reposting with
the review tag added (and corrected)? Thanks!

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

[Dmitry Vyukov]

On Tue, 15 Nov 2022 at 01:36, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 14 Nov 2022 19:27:29 +0900 Bongsu Jeon wrote:
> > Reviewed-by: Bongsu Jeon
>
> Dmitry if the patch is good after all - would you mind reposting with
> the review tag added (and corrected)? Thanks!

Done:
https://lore.kernel.org/all/20221115100017.787929-1-dvyukov@google.com/

Also sent a patch that adds "send after close" case:
https://lore.kernel.org/all/20221115095941.787250-1-dvyukov@google.com/

(these patches can be merged independently)