can: virtio: Add virtio CAN driver

drivers/net/can/virtio_can.c line 831

...
We're here clearly beyond column 80 with something which is not a string or a comment. Elsewhere we are also sometimes beyond column 80 but then only 1 or 2 characters which may or may not be acceptable by the kernel community. No idea. You may re-check the formatting.

Besides formatting:

I compared your actual v10 of the virtio CAN driver against my last v5 and and I saw nothing in your code which may cause a problem. Only learned doing the review this way that at some corners my old v5 code was not this bug free as I thought when I saw some changes. Nothing serious because in some "not expected to happen" corner case code paths but anyway I'll occasionally have to take over some of your changes into an internal project repository here.

We will see whether this virtio CAN device will be accepted in some near future for integration in the mainline kernel...

Regards
Harald

drivers/net/can/Kconfig config CAN_XILINXCAN · line 239

The CAN_VIRTIO_CAN config entry is placed after CAN_XILINXCAN, breaking the alphabetical ordering maintained by all other config entries in this file. Since V comes before X alphabetically, should CAN_VIRTIO_CAN be placed between CAN_TI_HECC and CAN_XILINXCAN? The Makefile entry is correctly ordered (virtio_can.o appears before xilinx_can.o), which makes this inconsistency more evident.

drivers/net/can/virtio_can.c line 117

Is GFP_KERNEL safe here? This function is called from virtio_can_start_xmit(), which is the ndo_start_xmit callback. This callback can be invoked from softirq context with BH disabled, where sleeping is not allowed. Should this use GFP_ATOMIC to match the calling context?

drivers/net/can/virtio_can.c line 226

Is open_candev() missing? The CAN framework requires drivers to call open_candev() in their ndo_open handler. open_candev() validates that bittiming has been configured (returns -EINVAL if bitrate is 0), checks CAN FD data bitrate requirements, and manages carrier state. Without this call, can the device be brought up without configuring bitrate? The driver does call close_candev() in virtio_can_close(), showing awareness of the CAN lifecycle. Every other CAN driver in the kernel calls open_candev() in its open function.

[ ... ]

drivers/net/can/virtio_can.c line 283

Does this leak the skb? When can_dev_dropped_skb() returns false (valid frame, skb not consumed) and kzalloc() for can_tx_msg fails, the code jumps to 'kick' without freeing the skb. The skb is still owned by the caller at this point. Should kfree_skb(skb) be called before goto kick?

drivers/net/can/virtio_can.c line 317

Does this also leak the skb? When ida_alloc_max() returns -ENOMEM, can_tx_msg is correctly freed but the skb is not. can_put_echo_skb() has not been called yet at this point, so the skb is still owned by virtio_can_start_xmit. Should kfree_skb(skb) be called before goto kick?

drivers/net/can/virtio_can.c line 704

Does this cause an out-of-bounds memory access? 'buf' is declared as 'struct virtio_can_rx *' and 'buf_size' equals 'priv->rpkt_len' (sizeof(struct virtio_can_rx) + CAN[FD]_MAX_DLEN). The expression 'buf += buf_size' advances by buf_size * sizeof(struct virtio_can_rx) bytes instead of the intended buf_size bytes, because C pointer arithmetic scales by the pointed-to type size.

For CAN classic (rpkt_len=24, sizeof(struct virtio_can_rx)=16), this advances 384 bytes per iteration instead of 24. The allocation in virtio_can_probe only provides rpkt_len bytes per element, so doesn't the second and all subsequent loop iterations pass out-of-bounds pointers to the virtqueue as RX buffers? When the device writes to these buffers, can this corrupt adjacent kernel memory?

virtio_can_probe():

    priv->rpkt = devm_kzalloc(&vdev->dev, priv->rpkt_len *
                                priv->vqs[VIRTIO_CAN_QUEUE_RX]->num_free,
                                GFP_KERNEL);

Should a 'char *' or 'u8 *' pointer be used for the byte-level iteration instead, e.g. 'u8 *p = (u8 *)priv->rpkt; ... p += buf_size;' and pass 'p' to virtio_can_add_inbuf?

drivers/net/can/virtio_can.c

Does this use an uninitialized variable? The variable 'ret' is declared as 'int ret;' without initialization. If vq->num_free equals 0, the for loop body never executes, and 'ret' is read uninitialized here. Should this be initialized to 0?

drivers/net/can/virtio_can.c line 733

Should this use virtio_reset_device() instead? When CONFIG_VIRTIO_HARDEN_NOTIFICATION is enabled, virtio_reset_device() calls virtio_break_device() to mark all virtqueues as broken and virtio_synchronize_cbs() to wait for in-flight callbacks to complete before performing the actual reset. Calling config->reset directly skips these hardening steps, meaning virtqueue callbacks may still be executing when del_vqs destroys the queues. All other virtio network drivers (virtio_net.c, caif_virtio.c, mac80211_hwsim.c) use virtio_reset_device(). The NULL check is also unnecessary as this op is always present in virtio devices.

drivers/net/can/virtio_can.c line 754

Are napi_disable() calls missing here? The probe function enables both NAPI instances:

virtio_can_probe():

    napi_enable(&priv->napi);
    napi_enable(&priv->napi_tx);

And virtio_can_freeze() correctly disables them:

virtio_can_freeze():

    napi_disable(&priv->napi);
    napi_disable(&priv->napi_tx);

But virtio_can_remove() never disables NAPI. The ndo_stop callback (virtio_can_close) intentionally leaves NAPI active per its comment. Can the NAPI poll callbacks (virtio_can_rx_poll, virtio_can_tx_poll) execute concurrently with virtio_can_del_vq() deleting virtqueues and virtio_can_free_candev() freeing the net_device? Should napi_disable() be called for both instances before unregister_candev()?

drivers/net/can/virtio_can.c line 757

[ ... ]
--

Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |

Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |

drivers/net/can/virtio_can.c line 195

propagate the error value and evaluate it in the caller

[...]

drivers/net/can/virtio_can.c line 236

propagate the error value and evaluate it in the caller

drivers/net/can/virtio_can.c line 244

regards,
Marc

--

Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |

Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |