linux-kernel - Re: [PATCH v5] can: virtio: Initial virtio CAN driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <27327622.1r3eYUQgxm@fedora.fritz.box>
Date: Mon, 20 Oct 2025 23:24:15 +0200
From: Francesco Valla <francesco@...la.it>
To: Matias Ezequiel Vara Larsen <mvaralar@...hat.com>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, Paolo Abeni <pabeni@...hat.com>,
 Harald Mommer <harald.mommer@...nsynergy.com>,
 Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com>,
 Wolfgang Grandegger <wg@...ndegger.com>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
 Jason Wang <jasowang@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
 Damir Shaikhutdinov <Damir.Shaikhutdinov@...nsynergy.com>,
 linux-kernel@...r.kernel.org, linux-can@...r.kernel.org,
 netdev@...r.kernel.org, virtualization@...ts.linux.dev,
 development@...aril.me
Subject: Re: [PATCH v5] can: virtio: Initial virtio CAN driver.

On Monday, 20 October 2025 at 16:56:08 Matias Ezequiel Vara Larsen <mvaralar@...hat.com> wrote:
> On Tue, Oct 14, 2025 at 06:01:07PM +0200, Francesco Valla wrote:
> > On Tuesday, 14 October 2025 at 12:15:12 Matias Ezequiel Vara Larsen <mvaralar@...hat.com> wrote:
> > > On Thu, Sep 11, 2025 at 10:59:40PM +0200, Francesco Valla wrote:
> > > > Hello Mikhail, Harald,
> > > > 
> > > > hoping there will be a v6 of this patch soon, a few comments:
> > > > 
> > > > On Monday, 8 January 2024 at 14:10:35 Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com> wrote:
> > > > 
> > > > [...]
> > > > > +
> > > > > +/* Compare with m_can.c/m_can_echo_tx_event() */
> > > > > +static int virtio_can_read_tx_queue(struct virtqueue *vq)
> > > > > +{
> > > > > +	struct virtio_can_priv *can_priv = vq->vdev->priv;
> > > > > +	struct net_device *dev = can_priv->dev;
> > > > > +	struct virtio_can_tx *can_tx_msg;
> > > > > +	struct net_device_stats *stats;
> > > > > +	unsigned long flags;
> > > > > +	unsigned int len;
> > > > > +	u8 result;
> > > > > +
> > > > > +	stats = &dev->stats;
> > > > > +
> > > > > +	/* Protect list and virtio queue operations */
> > > > > +	spin_lock_irqsave(&can_priv->tx_lock, flags);
> > > > > +
> > > > > +	can_tx_msg = virtqueue_get_buf(vq, &len);
> > > > > +	if (!can_tx_msg) {
> > > > > +		spin_unlock_irqrestore(&can_priv->tx_lock, flags);
> > > > > +		return 0; /* No more data */
> > > > > +	}
> > > > > +
> > > > > +	if (unlikely(len < sizeof(struct virtio_can_tx_in))) {
> > > > > +		netdev_err(dev, "TX ACK: Device sent no result code\n");
> > > > > +		result = VIRTIO_CAN_RESULT_NOT_OK; /* Keep things going */
> > > > > +	} else {
> > > > > +		result = can_tx_msg->tx_in.result;
> > > > > +	}
> > > > > +
> > > > > +	if (can_priv->can.state < CAN_STATE_BUS_OFF) {
> > > > > +		/* Here also frames with result != VIRTIO_CAN_RESULT_OK are
> > > > > +		 * echoed. Intentional to bring a waiting process in an upper
> > > > > +		 * layer to an end.
> > > > > +		 * TODO: Any better means to indicate a problem here?
> > > > > +		 */
> > > > > +		if (result != VIRTIO_CAN_RESULT_OK)
> > > > > +			netdev_warn(dev, "TX ACK: Result = %u\n", result);
> > > > 
> > > > Maybe an error frame reporting CAN_ERR_CRTL_UNSPEC would be better?
> > > > 
> > > I am not sure. In xilinx_can.c, CAN_ERR_CRTL_UNSPEC is indicated during
> > > a problem in the rx path and this is the tx path. I think the comment
> > > refers to improving the way the driver informs this error to the user
> > > but I may be wrong.
> > > 
> > 
> > Since we have no detail of what went wrong here, I suggested
> > CAN_ERR_CRTL_UNSPEC as it is "unspecified error", to be coupled with a
> > controller error with id CAN_ERR_CRTL; however, a different error might be
> > more appropriate.
> > 
> > For sure, at least in my experience, having a warn printed to kmsg is *not*
> > enough, as the application sending the message(s) would not be able to detect
> > the error.
> > 
> > 
> > > > For sure, counting the known errors as valid tx_packets and tx_bytes
> > > > is misleading.
> > > > 
> > > 
> > > I'll remove the counters below.
> > > 
> > 
> > We don't really know what's wrong here - the packet might have been sent and
> > and then not ACK'ed, as well as any other error condition (as it happens in the
> > reference implementation from the original authors [1]). Echoing the packet
> > only "to bring a waiting process in an upper layer to an end" and incrementing
> > counters feels wrong, but maybe someone more expert than me can advise better
> > here.
> > 
> > 
> 
> I agree. IIUC, in case there has been a problem during transmission, I
> should 1) indicate this by injecting a CAN_ERR_CRTL_UNSPEC package with
> netif_rx() and 2) use can_free_echo_skb() and increment the tx_error
> stats. Is this correct?
> 
> Matias
> 
> 

That's my understanding too! stats->tx_dropped should be the right value to
increment (see for example [1]).

[1] https://elixir.bootlin.com/linux/v6.17.3/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L1035

Regards,
Francesco