| 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
| ||
|
Message-ID: <20250709111822.3b204b27@wsk>
Date: Wed, 9 Jul 2025 11:18:22 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, davem@...emloft.net, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
<s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, Richard Cochran
<richardcochran@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, Stefan Wahren
<wahrenst@....net>, Simon Horman <horms@...nel.org>
Subject: Re: [net-next v14 07/12] net: mtip: Add mtip_switch_{rx|tx}
functions to the L2 switch driver
Hi Paolo,
> On 7/1/25 1:49 PM, Lukasz Majewski wrote:
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c index
> > 63afdf2beea6..b5a82748b39b 100644 ---
> > a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c +++
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c @@ -228,6
> > +228,39 @@ struct mtip_port_info *mtip_portinfofifo_read(struct
> > switch_enet_private *fep) return info; }
> >
> > +static void mtip_atable_get_entry_port_number(struct
> > switch_enet_private *fep,
> > + unsigned char
> > *mac_addr, u8 *port) +{
> > + int block_index, block_index_end, entry;
> > + u32 mac_addr_lo, mac_addr_hi;
> > + u32 read_lo, read_hi;
> > +
> > + mac_addr_lo = (u32)((mac_addr[3] << 24) | (mac_addr[2] <<
> > 16) |
> > + (mac_addr[1] << 8) | mac_addr[0]);
> > + mac_addr_hi = (u32)((mac_addr[5] << 8) | (mac_addr[4]));
> > +
> > + block_index = GET_BLOCK_PTR(crc8_calc(mac_addr));
> > + block_index_end = block_index + ATABLE_ENTRY_PER_SLOT;
> > +
> > + /* now search all the entries in the selected block */
> > + for (entry = block_index; entry < block_index_end;
> > entry++) {
> > + mtip_read_atable(fep, entry, &read_lo, &read_hi);
> > + *port = MTIP_PORT_FORWARDING_INIT;
> > +
> > + if (read_lo == mac_addr_lo &&
> > + ((read_hi & 0x0000FFFF) ==
> > + (mac_addr_hi & 0x0000FFFF))) {
> > + /* found the correct address */
> > + if ((read_hi & (1 << 16)) && (!(read_hi &
> > (1 << 17))))
> > + *port = FIELD_GET(AT_PORT_MASK,
> > read_hi);
> > + break;
> > + }
> > + }
> > +
> > + dev_dbg(&fep->pdev->dev, "%s: MAC: %pM PORT: 0x%x\n",
> > __func__,
> > + mac_addr, *port);
> > +}
> > +
> > /* Clear complete MAC Look Up Table */
> > void mtip_clear_atable(struct switch_enet_private *fep)
> > {
> > @@ -825,11 +858,217 @@ static irqreturn_t mtip_interrupt(int irq,
> > void *ptr_fep)
> > static void mtip_switch_tx(struct net_device *dev)
> > {
> > + struct mtip_ndev_priv *priv = netdev_priv(dev);
> > + struct switch_enet_private *fep = priv->fep;
> > + unsigned short status;
> > + struct sk_buff *skb;
> > + struct cbd_t *bdp;
> > +
> > + spin_lock(&fep->hw_lock);
> > + bdp = fep->dirty_tx;
> > +
> > + while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
> > + if (bdp == fep->cur_tx && fep->tx_full == 0)
> > + break;
> > +
> > + dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
> > + MTIP_SWITCH_TX_FRSIZE,
> > DMA_TO_DEVICE);
> > + bdp->cbd_bufaddr = 0;
> > + skb = fep->tx_skbuff[fep->skb_dirty];
> > + /* Check for errors */
> > + if (status & (BD_ENET_TX_HB | BD_ENET_TX_LC |
> > + BD_ENET_TX_RL | BD_ENET_TX_UN |
> > + BD_ENET_TX_CSL)) {
> > + dev->stats.tx_errors++;
> > + if (status & BD_ENET_TX_HB) /* No
> > heartbeat */
> > + dev->stats.tx_heartbeat_errors++;
> > + if (status & BD_ENET_TX_LC) /* Late
> > collision */
> > + dev->stats.tx_window_errors++;
> > + if (status & BD_ENET_TX_RL) /* Retrans
> > limit */
> > + dev->stats.tx_aborted_errors++;
> > + if (status & BD_ENET_TX_UN) /* Underrun */
> > + dev->stats.tx_fifo_errors++;
> > + if (status & BD_ENET_TX_CSL) /* Carrier
> > lost */
> > + dev->stats.tx_carrier_errors++;
> > + } else {
> > + dev->stats.tx_packets++;
> > + }
> > +
> > + if (status & BD_ENET_TX_READY)
> > + dev_err(&fep->pdev->dev,
> > + "Enet xmit interrupt and
> > TX_READY.\n"); +
> > + /* Deferred means some collisions occurred during
> > transmit,
> > + * but we eventually sent the packet OK.
> > + */
> > + if (status & BD_ENET_TX_DEF)
> > + dev->stats.collisions++;
> > +
> > + /* Free the sk buffer associated with this last
> > transmit */
> > + dev_consume_skb_irq(skb);
> > + fep->tx_skbuff[fep->skb_dirty] = NULL;
> > + fep->skb_dirty = (fep->skb_dirty + 1) &
> > TX_RING_MOD_MASK; +
> > + /* Update pointer to next buffer descriptor to be
> > transmitted */
> > + if (status & BD_ENET_TX_WRAP)
> > + bdp = fep->tx_bd_base;
> > + else
> > + bdp++;
> > +
> > + /* Since we have freed up a buffer, the ring is no
> > longer
> > + * full.
> > + */
> > + if (fep->tx_full) {
> > + fep->tx_full = 0;
> > + if (netif_queue_stopped(dev))
> > + netif_wake_queue(dev);
> > + }
> > + }
> > + fep->dirty_tx = bdp;
> > + spin_unlock(&fep->hw_lock);
> > }
> >
> > +/* During a receive, the cur_rx points to the current incoming
> > buffer.
> > + * When we update through the ring, if the next incoming buffer has
> > + * not been given to the system, we just set the empty indicator,
> > + * effectively tossing the packet.
> > + */
> > static int mtip_switch_rx(struct net_device *dev, int budget, int
> > *port) {
> > - return -ENOMEM;
> > + struct mtip_ndev_priv *priv = netdev_priv(dev);
> > + u8 *data, rx_port = MTIP_PORT_FORWARDING_INIT;
> > + struct switch_enet_private *fep = priv->fep;
> > + unsigned short status, pkt_len;
> > + struct net_device *pndev;
> > + struct ethhdr *eth_hdr;
> > + int pkt_received = 0;
> > + struct sk_buff *skb;
> > + struct cbd_t *bdp;
> > + struct page *page;
> > +
> > + /* First, grab all of the stats for the incoming packet.
> > + * These get messed up if we get called due to a busy
> > condition.
> > + */
> > + bdp = fep->cur_rx;
> > +
> > + while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {
> > + if (pkt_received >= budget)
> > + break;
> > +
> > + pkt_received++;
> > +
> > + if (!fep->usage_count)
> > + goto rx_processing_done;
> > +
> > + status ^= BD_ENET_RX_LAST;
> > + /* Check for errors. */
> > + if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH |
> > BD_ENET_RX_NO |
> > + BD_ENET_RX_CR | BD_ENET_RX_OV |
> > BD_ENET_RX_LAST |
> > + BD_ENET_RX_CL)) {
> > + dev->stats.rx_errors++;
> > + if (status & BD_ENET_RX_OV) {
> > + /* FIFO overrun */
> > + dev->stats.rx_fifo_errors++;
> > + goto rx_processing_done;
> > + }
> > + if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH
> > + | BD_ENET_RX_LAST)) {
> > + /* Frame too long or too short. */
> > + dev->stats.rx_length_errors++;
> > + if (status & BD_ENET_RX_LAST)
> > + netdev_err(dev, "rcv is
> > not +last\n");
> > + }
> > + if (status & BD_ENET_RX_CR) /* CRC
> > Error */
> > + dev->stats.rx_crc_errors++;
> > +
> > + /* Report late collisions as a frame
> > error. */
> > + if (status & (BD_ENET_RX_NO |
> > BD_ENET_RX_CL))
> > + dev->stats.rx_frame_errors++;
> > + goto rx_processing_done;
> > + }
> > +
> > + /* Get correct RX page */
> > + page = fep->page[bdp - fep->rx_bd_base];
> > + /* Process the incoming frame */
> > + pkt_len = bdp->cbd_datlen;
> > + data = (__u8 *)__va(bdp->cbd_bufaddr);
> > +
> > + dma_sync_single_for_cpu(&fep->pdev->dev,
> > bdp->cbd_bufaddr,
> > + pkt_len, DMA_FROM_DEVICE);
> > + net_prefetch(page_address(page));
>
> Both `__va(bdp->cbd_bufaddr)` and `page_address(page)` should point to
> the same same memory. Please use constantly one _or_ the other -
> likely page_address(page) is the best option.
Yes, the __va() can be replaced with page_address(page) as those are
the same addresses.
>
> > +
> > + if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> > + swap_buffer(data, pkt_len);
> > +
> > + if (data) {
>
> The above check is not needed. If data is null swap_buffer will still
> unconditionally dereference it.
+1
>
> Also it looks like it can't be NULL.
Yes, those buffers are allocated at the initialization phase - if the
allocation fails, driver is not operational.
>
> /P
>
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Johanna Denk,
Tabea Lutz HRB 165235 Munich, Office: Kirchenstr.5, D-82194
Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists