netdev - RE: [EXTERNAL] Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC

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: 
 <MWHPR1801MB191837C8907B39F67893F0BBD3212@MWHPR1801MB1918.namprd18.prod.outlook.com>
Date: Wed, 6 Mar 2024 16:01:48 +0000
From: Ratheesh Kannoth <rkannoth@...vell.com>
To: Herve Codina <herve.codina@...tlin.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC

> From: Herve Codina <herve.codina@...tlin.com>
> Sent: Wednesday, March 6, 2024 7:08 PM
> To: Ratheesh Kannoth <rkannoth@...vell.com>
> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC
> 
> > > +static void qmc_hcld_recv_complete(void *context, size_t length,
> > > +unsigned int flags) {
> > > +	struct qmc_hdlc_desc *desc = context;
> > > +	struct net_device *netdev = desc->netdev;
> > > +	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> > Reverse xmas tree
> 
> The reverse xmas tree order cannot be used here.
> qmc_hdlc depends on netdev, netdev depends on desc.
> 
ACK. Usually I get comments to split declaration and assignment for my patches in upstream. 

> ...
> > > +static void qmc_hdlc_xmit_complete(void *context)
> > > +{
> > > +	struct qmc_hdlc_desc *desc = context;
> > > +	struct net_device *netdev = desc->netdev;
> > > +	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> > > +	struct sk_buff *skb;
> > Reverse xmas tree
> 
> Ditto
Same as above. 

> 
> > > +
> > > +	scoped_guard(spinlock_irqsave, &qmc_hdlc->tx_lock) {
> > > +		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc-
> >dma_size, DMA_TO_DEVICE);
> > > +		skb = desc->skb;
> > > +		desc->skb = NULL; /* Release the descriptor */
> > > +		if (netif_queue_stopped(netdev))
> > > +			netif_wake_queue(netdev);
> > > +	}
> > > +
> > > +	netdev->stats.tx_packets++;
> > > +	netdev->stats.tx_bytes += skb->len;
> > > +
> > > +	dev_consume_skb_any(skb);
> > > +}
> > > +
> ...
> > > +
> > > +static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, struct net_device
> *netdev)
> > > +{
> > > +	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> > > +	struct qmc_hdlc_desc *desc;
> > > +	int err;
> > > +
> > > +	guard(spinlock_irqsave)(&qmc_hdlc->tx_lock);
> > > +
> > > +	desc = &qmc_hdlc->tx_descs[qmc_hdlc->tx_out];
> > > +	if (WARN_ONCE(desc->skb, "No tx descriptors available\n")) {
> > > +		/* Should never happen.
> > > +		 * Previous xmit should have already stopped the queue.
> > > +		 */
> > > +		netif_stop_queue(netdev);
> > > +		return NETDEV_TX_BUSY;
> > > +	}
> > > +
> > > +	desc->netdev = netdev;
> > > +	desc->dma_size = skb->len;
> > > +	desc->skb = skb;
> > > +	err = qmc_hdlc_xmit_queue(qmc_hdlc, desc);
> > > +	if (err) {
> > > +		desc->skb = NULL; /* Release the descriptor */
> > > +		if (err == -EBUSY) {
> > > +			netif_stop_queue(netdev);
> > > +			return NETDEV_TX_BUSY;
> > > +		}
> > > +		dev_kfree_skb(skb);
> > > +		netdev->stats.tx_dropped++;
> > > +		return NETDEV_TX_OK;
> > > +	}
> > > +
> > > +	qmc_hdlc->tx_out = (qmc_hdlc->tx_out + 1) % ARRAY_SIZE(qmc_hdlc-
> >tx_descs);
> > > +
> > > +	if (qmc_hdlc->tx_descs[qmc_hdlc->tx_out].skb)
> > wont it race if tx completion and this function context run in different cpu ?
> 
> We are protected by the qmc_hdlc->tx_lock spinlock.
> 
> guard() call in this function and scoped_guard() call in
> qmc_hdlc_xmit_complete().
> 
> What is the race you thought of ?
ACK. Thanks for clarification. 


> 
> >
> > > +		netif_stop_queue(netdev);
> > > +
> > > +	return NETDEV_TX_OK;
> > > +}
> > > +
> ...
> > > +	/* Queue as many recv descriptors as possible */
> > > +	for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
> > > +		desc = &qmc_hdlc->rx_descs[i];
> > > +
> > > +		desc->netdev = netdev;
> > > +		ret = qmc_hdlc_recv_queue(qmc_hdlc, desc,
> chan_param.hdlc.max_rx_buf_size);
> > > +		if (ret == -EBUSY && i != 0)
> > > +			break; /* We use all the QMC chan capability */
> > > +		if (ret)
> > > +			goto free_desc;
> > > +	}
> > > +
> > > +	ret = qmc_chan_start(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
> > > +	if (ret) {
> > > +		dev_err(qmc_hdlc->dev, "qmc chan start failed (%d)\n", ret);
> > > +		goto free_desc;
> > > +	}
> > > +
> > > +	netif_start_queue(netdev);
> > > +
> > > +	return 0;
> > > +
> > > +free_desc:
> > > +	qmc_chan_reset(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
> > > +	while (i--) {
> > Double free ? i'th descriptor skb is freed in qmc_hdlc_recv_queue()
> function's error handler itself.
> > Should we be predecrement of i ?
> 
> Suppose a failure on i = 5. The item 5 is already cleaned (done by
> qmc_hdlc_recv_queue() itself).
> The 'while (i--)' set i to 4 and operations are performed with i = 4, 3, 2, 1 and 0.
> 
> Where is the double free ?
> Do I miss something ?
My bad. ACK. 


> >
> > > +		desc = &qmc_hdlc->rx_descs[i];
> > > +		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc-
> >dma_size,
> > > +				 DMA_FROM_DEVICE);
> > > +		kfree_skb(desc->skb);
> > > +		desc->skb = NULL;
> > > +	}
> > > +hdlc_close:
> > > +	hdlc_close(netdev);
> > > +	return ret;
> > > +}
> > > +
> > > +
> 
> Best regards,
> Hervé