Re: [PATCH] nfc: Allow to create multiple virtual nci devices

[Leon Romanovsky <leon@kernel.org>]

On Mon, Oct 31, 2022 at 08:36:57AM -0700, Dmitry Vyukov wrote:
> On Mon, 31 Oct 2022 at 02:23, Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sun, Oct 30, 2022 at 03:29:19PM +0100, Dmitry Vyukov 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
> > > ---
> > >  drivers/nfc/virtual_ncidev.c | 143 ++++++++++++++++-------------------
> > >  1 file changed, 66 insertions(+), 77 deletions(-)
> >
> > <...>
> >
> > >  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);
> > > -             kfree_skb(skb);
> > > -             return 0;
> > > -     }
> > > +     struct virtual_nci_dev *vdev = nci_get_drvdata(ndev);
> > >
> > > -     if (send_buff) {
> > > -             mutex_unlock(&nci_mutex);
> > > +     mutex_lock(&vdev->mtx);
> > > +     if (vdev->send_buff) {
> > > +             mutex_unlock(&vdev->mtx);
> > >               kfree_skb(skb);
> >
> > You probably need to set vdev->send_buff to NULL here.
> 
> Hi Leon,
> 
> Thanks for looking at this.
> 
> Are you sure about setting vdev->send_buff to NULL?
> We already have a "cached" skb in vdev->send_buff, we received a new
> one in 'skb' and freed it.
> I assumed the intention is to keep vdev->send_buff intact.

You are right.

> 
> > >               return -1;
> > >       }
> > > -     send_buff = skb_copy(skb, GFP_KERNEL);
> > > -     mutex_unlock(&nci_mutex);
> > > -     wake_up_interruptible(&wq);
> > > +     vdev->send_buff = skb_copy(skb, GFP_KERNEL);
> >
> > You don't check return value of skb_copy(), it can fail, but
> > this function will return 0 (success). Do you do it deliberately?
> >
> > If yes, please add a comment to the code, as it is not clear.
> 
> Good question. I just kept all of this logic as it is now and only
> removed the global vars.

I know :)

> 
> I guess we need something like this, right?
> 
> vdev->send_buff = skb_copy(skb, GFP_KERNEL);
> if (!vdev->send_buff) {
>     mutex_unlock(&vdev->mtx);
>     return -1;
> }
> 
> Though, it's called only from nci_send_frame() and its return value is
> never checked :)

I would say that the most important part is do not continue after
skb_copy() failure.

Thanks

> 
> $ git grep nci_send_frame
> include/net/nfc/nci_core.h:int nci_send_frame(struct nci_dev *ndev,
> struct sk_buff *skb);
> net/nfc/nci/core.c:int nci_send_frame(struct nci_dev *ndev, struct sk_buff *skb)
> net/nfc/nci/core.c:EXPORT_SYMBOL(nci_send_frame);
> drivers/nfc/nfcmrvl/fw_dnld.c:
> nci_send_frame(priv->ndev, out_skb);
> drivers/nfc/nfcmrvl/fw_dnld.c:          nci_send_frame(priv->ndev, out_skb);
> drivers/nfc/nfcmrvl/fw_dnld.c:
> nci_send_frame(priv->ndev, out_skb);
> net/nfc/nci/core.c:             nci_send_frame(ndev, skb);
> net/nfc/nci/core.c:             nci_send_frame(ndev, skb);
> 
> 
> > Thanks
> >
> > > +     mutex_unlock(&vdev->mtx);
> > > +     wake_up_interruptible(&vdev->wq);
> > >       consume_skb(skb);
> > >
> > >       return 0;