On Mon, 31 Oct 2022 at 02:23, Leon Romanovsky [off-list ref] wrote:

On Sun, Oct 30, 2022 at 03:29:19PM +0100, Dmitry Vyukov wrote:

quoted

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 <redacted>
Cc: netdev@vger.kernel.org
---
 drivers/nfc/virtual_ncidev.c | 143 ++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 77 deletions(-)

<...>

quoted

 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.

quoted

              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 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 :)

$ 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

quoted

+     mutex_unlock(&vdev->mtx);
+     wake_up_interruptible(&vdev->wq);
      consume_skb(skb);

      return 0;