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

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

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