| 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: <20220322210402.ebr2zghcisrqz4ju@soft-dev3-1.localhost>
Date: Tue, 22 Mar 2022 22:04:02 +0100
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
<davem@...emloft.net>, <michael@...le.cc>,
<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next v2 3/4] net: lan966x: Add FDMA functionality
The 03/21/2022 23:01, Jakub Kicinski wrote:
Hi Jakub,
Thanks for the review.
Please see bellow my comments.
>
> On Fri, 18 Mar 2022 21:47:49 +0100 Horatiu Vultur wrote:
> > Ethernet frames can be extracted or injected to or from the device's
> > DDR memory. There is one channel for injection and one channel for
> > extraction. Each of these channels contain a linked list of DCBs which
> > contains DB. The DCB for injection contains only 1 DB and the DCB for
> > extraction contains 3 DBs. Each DB contains a frame. Everytime when a
> > frame is received or transmitted an interrupt is generated.
> >
> > It is not possible to use both the FDMA and the manual
> > injection/extraction of the frames. Therefore the FDMA has priority over
> > the manual because of better performance values.
>
>
> > +static struct sk_buff *lan966x_fdma_rx_alloc_skb(struct lan966x_rx *rx,
> > + struct lan966x_db *db)
> > +{
> > + struct lan966x *lan966x = rx->lan966x;
> > + struct sk_buff *skb;
> > + dma_addr_t dma_addr;
> > + struct page *page;
> > + void *buff_addr;
> > +
> > + page = dev_alloc_pages(rx->page_order);
> > + if (unlikely(!page))
> > + return NULL;
> > +
> > + dma_addr = dma_map_page(lan966x->dev, page, 0,
> > + PAGE_SIZE << rx->page_order,
> > + DMA_FROM_DEVICE);
> > + if (unlikely(dma_mapping_error(lan966x->dev, dma_addr))) {
> > + __free_pages(page, rx->page_order);
> > + return NULL;
> > + }
> > +
> > + buff_addr = page_address(page);
> > + skb = build_skb(buff_addr, PAGE_SIZE << rx->page_order);
> > +
>
> spurious new line
>
> > + if (unlikely(!skb)) {
> > + dev_err_ratelimited(lan966x->dev,
> > + "build_skb failed !\n");
>
> a statistic for that would be better than prints
> kernel will already do some printing if it goes oom
>
> > + dma_unmap_single(lan966x->dev, dma_addr,
> > + PAGE_SIZE << rx->page_order,
> > + DMA_FROM_DEVICE);
>
> why unmap_single if you used map_page for mapping?
That is a mistake. I will fix in the next version.
>
> > + __free_pages(page, rx->page_order);
> > + return NULL;
>
> please move the error handling to the end of the function and use goto
> to jump into the right spot
>
> > + }
> > +
> > + db->dataptr = dma_addr;
> > + return skb;
> > +}
>
> > +static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
> > +{
> > + struct lan966x *lan966x = rx->lan966x;
> > + struct lan966x_rx_dcb *dcb;
> > + struct lan966x_db *db;
> > + struct sk_buff *skb;
> > + int i, j;
> > + int size;
> > +
> > + /* calculate how many pages are needed to allocate the dcbs */
> > + size = sizeof(struct lan966x_rx_dcb) * FDMA_DCB_MAX;
> > + size = ALIGN(size, PAGE_SIZE);
> > +
> > + rx->dcbs = dma_alloc_coherent(lan966x->dev, size, &rx->dma, GFP_ATOMIC);
>
> Why ATOMIC? This seems to be called from the probe path.
> Error checking missing.
The ATOMIC is a mistake for rx.
>
> > + rx->last_entry = rx->dcbs;
> > + rx->db_index = 0;
> > + rx->dcb_index = 0;
>
> > +static void lan966x_fdma_rx_start(struct lan966x_rx *rx)
> > +{
> > + struct lan966x *lan966x = rx->lan966x;
> > + u32 mask;
> > +
> > + /* When activating a channel, first is required to write the first DCB
> > + * address and then to activate it
> > + */
> > + lan_wr(((u64)rx->dma) & GENMASK(31, 0), lan966x,
>
> lower_32_bits()
>
> > + FDMA_DCB_LLP(rx->channel_id));
> > + lan_wr(((u64)rx->dma) >> 32, lan966x, FDMA_DCB_LLP1(rx->channel_id));
>
> upper_32_bits()
>
> > + lan_wr(FDMA_CH_CFG_CH_DCB_DB_CNT_SET(FDMA_RX_DCB_MAX_DBS) |
> > + FDMA_CH_CFG_CH_INTR_DB_EOF_ONLY_SET(1) |
> > + FDMA_CH_CFG_CH_INJ_PORT_SET(0) |
> > + FDMA_CH_CFG_CH_MEM_SET(1),
> > + lan966x, FDMA_CH_CFG(rx->channel_id));
>
> > +static int lan966x_fdma_tx_alloc(struct lan966x_tx *tx)
> > +{
> > + struct lan966x *lan966x = tx->lan966x;
> > + struct lan966x_tx_dcb *dcb;
> > + struct lan966x_db *db;
> > + int size;
> > + int i, j;
> > +
> > + tx->dcbs_buf = kcalloc(FDMA_DCB_MAX, sizeof(struct lan966x_tx_dcb_buf),
> > + GFP_ATOMIC);
> > + if (!tx->dcbs_buf)
> > + return -ENOMEM;
> > +
> > + /* calculate how many pages are needed to allocate the dcbs */
> > + size = sizeof(struct lan966x_tx_dcb) * FDMA_DCB_MAX;
> > + size = ALIGN(size, PAGE_SIZE);
> > + tx->dcbs = dma_alloc_coherent(lan966x->dev, size, &tx->dma, GFP_ATOMIC);
>
> same comments re: atomic and error checking as on rx
Here the ATOMIC is required because in patch 4, when changing the MTU,
we take a spin lock.
>
> > + /* Now for each dcb allocate the db */
> > + for (i = 0; i < FDMA_DCB_MAX; ++i) {
> > + dcb = &tx->dcbs[i];
> > +
> > + for (j = 0; j < FDMA_TX_DCB_MAX_DBS; ++j) {
> > + db = &dcb->db[j];
> > + db->dataptr = 0;
> > + db->status = 0;
> > + }
> > +
> > + lan966x_fdma_tx_add_dcb(tx, dcb);
> > + }
> > +
> > + return 0;
> > +}
>
> > +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> > +{
> > + struct lan966x *lan966x = rx->lan966x;
> > + u64 src_port, timestamp;
> > + struct sk_buff *new_skb;
> > + struct lan966x_db *db;
> > + struct sk_buff *skb;
> > +
> > + /* Check if there is any data */
> > + db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
> > + if (unlikely(!(db->status & FDMA_DCB_STATUS_DONE)))
> > + return NULL;
> > +
> > + /* Get the received frame and unmap it */
> > + skb = rx->skb[rx->dcb_index][rx->db_index];
> > + dma_unmap_single(lan966x->dev, (dma_addr_t)db->dataptr,
> > + FDMA_DCB_STATUS_BLOCKL(db->status),
> > + DMA_FROM_DEVICE);
> > +
> > + /* Allocate a new skb and map it */
> > + new_skb = lan966x_fdma_rx_alloc_skb(rx, db);
> > + if (unlikely(!new_skb))
> > + return NULL;
>
> So how is memory pressure handled, exactly? Looks like it's handled
> the same as if the ring was empty, so the IRQ is going to get re-raise
> immediately, or never raised again?
That is correct, the IRQ is going to get re-raised.
But I am not sure that this is correct approach. Do you have any
suggestions how it should be?
>
> > + rx->skb[rx->dcb_index][rx->db_index] = new_skb;
> > +
> > + skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
> > +
> > + lan966x_ifh_get_src_port(skb->data, &src_port);
> > + lan966x_ifh_get_timestamp(skb->data, ×tamp);
> > +
> > + WARN_ON(src_port >= lan966x->num_phys_ports);
> > +
> > + skb->dev = lan966x->ports[src_port]->dev;
> > + skb_pull(skb, IFH_LEN * sizeof(u32));
> > +
> > + if (likely(!(skb->dev->features & NETIF_F_RXFCS)))
> > + skb_trim(skb, skb->len - ETH_FCS_LEN);
> > +
> > + lan966x_ptp_rxtstamp(lan966x, skb, timestamp);
> > + skb->protocol = eth_type_trans(skb, skb->dev);
> > +
> > + if (lan966x->bridge_mask & BIT(src_port)) {
> > + skb->offload_fwd_mark = 1;
> > +
> > + skb_reset_network_header(skb);
> > + if (!lan966x_hw_offload(lan966x, src_port, skb))
> > + skb->offload_fwd_mark = 0;
> > + }
> > +
> > + skb->dev->stats.rx_bytes += skb->len;
> > + skb->dev->stats.rx_packets++;
> > +
> > + return skb;
> > +}
> > +
> > +static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
> > +{
> > + struct lan966x *lan966x = container_of(napi, struct lan966x, napi);
> > + struct lan966x_rx *rx = &lan966x->rx;
> > + struct list_head rx_list;
> > + int counter = 0;
> > +
> > + lan966x_fdma_tx_clear_buf(lan966x, weight);
> > +
> > + INIT_LIST_HEAD(&rx_list);
> > +
> > + while (counter < weight) {
> > + struct lan966x_rx_dcb *old_dcb;
> > + struct sk_buff *skb;
> > + u64 nextptr;
> > +
> > + skb = lan966x_fdma_rx_get_frame(rx);
> > + if (!skb)
> > + break;
> > + list_add_tail(&skb->list, &rx_list);
> > +
> > + rx->db_index++;
> > + counter++;
> > +
> > + /* Check if the DCB can be reused */
> > + if (rx->db_index != FDMA_RX_DCB_MAX_DBS)
> > + continue;
> > +
> > + /* Now the DCB can be reused, just advance the dcb_index
> > + * pointer and set the nextptr in the DCB
> > + */
> > + rx->db_index = 0;
> > +
> > + old_dcb = &rx->dcbs[rx->dcb_index];
> > + rx->dcb_index++;
> > + rx->dcb_index &= FDMA_DCB_MAX - 1;
> > +
> > + nextptr = rx->dma + ((unsigned long)old_dcb -
> > + (unsigned long)rx->dcbs);
> > + lan966x_fdma_rx_add_dcb(rx, old_dcb, nextptr);
> > + lan966x_fdma_rx_reload(rx);
> > + }
> > +
> > + if (counter < weight) {
> > + napi_complete_done(napi, counter);
>
> You should check the return value of napi_complete_done().
> busy polling or something else may want the IRQ to stay
> disabled.
>
> > + lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
> > + }
> > +
> > + netif_receive_skb_list(&rx_list);
>
> Why not GRO?
No reason, I have just looked at other drivers what they are doing.
I will update this.
>
> > + return counter;
> > +}
> > +
> > +irqreturn_t lan966x_fdma_irq_handler(int irq, void *args)
> > +{
> > + struct lan966x *lan966x = args;
> > + u32 db, err, err_type;
> > +
> > + db = lan_rd(lan966x, FDMA_INTR_DB);
> > + err = lan_rd(lan966x, FDMA_INTR_ERR);
>
> Hm, IIUC you request a threaded IRQ for this. Why?
> The register accesses can't sleep because you poke
> them from napi_poll as well...
Good point. What about the WARN?
>
> > + if (db) {
> > + lan_wr(0, lan966x, FDMA_INTR_DB_ENA);
> > + lan_wr(db, lan966x, FDMA_INTR_DB);
> > +
> > + napi_schedule(&lan966x->napi);
> > + }
> > +
> > + if (err) {
> > + err_type = lan_rd(lan966x, FDMA_ERRORS);
> > +
> > + WARN(1, "Unexpected error: %d, error_type: %d\n", err, err_type);
> > +
> > + lan_wr(err, lan966x, FDMA_INTR_ERR);
> > + lan_wr(err_type, lan966x, FDMA_ERRORS);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
>
> > +int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
> > +{
> > + struct lan966x_port *port = netdev_priv(dev);
> > + struct lan966x *lan966x = port->lan966x;
> > + struct lan966x_tx_dcb_buf *next_dcb_buf;
> > + struct lan966x_tx_dcb *next_dcb, *dcb;
> > + struct lan966x_tx *tx = &lan966x->tx;
> > + struct lan966x_db *next_db;
> > + int needed_headroom;
> > + int needed_tailroom;
> > + dma_addr_t dma_addr;
> > + int next_to_use;
> > + int err;
> > +
> > + /* Get next index */
> > + next_to_use = lan966x_fdma_get_next_dcb(tx);
> > + if (next_to_use < 0) {
> > + netif_stop_queue(dev);
> > + err = NETDEV_TX_BUSY;
> > + goto out;
> > + }
> > +
> > + if (skb_put_padto(skb, ETH_ZLEN)) {
> > + dev->stats.tx_dropped++;
>
> It's preferred not to use the old dev->stats, but I guess you already
> do so :( This is under some locks, right? No chance for another queue
> or port to try to touch those stats at the same time?
What is the preffered way of doing it?
Yes, it is under a lock.
>
> > + err = NETDEV_TX_OK;
> > + goto out;
> > + }
> > +
> > + /* skb processing */
> > + needed_headroom = max_t(int, IFH_LEN * sizeof(u32) - skb_headroom(skb), 0);
> > + needed_tailroom = max_t(int, ETH_FCS_LEN - skb_tailroom(skb), 0);
> > + if (needed_headroom || needed_tailroom) {
> > + err = pskb_expand_head(skb, needed_headroom, needed_tailroom,
> > + GFP_ATOMIC);
> > + if (unlikely(err)) {
> > + dev->stats.tx_dropped++;
> > + err = NETDEV_TX_OK;
> > + goto release;
> > + }
> > + }
>
> You need to skb_cow_head() even if you don't need to grow the headroom
> or tailroom.
>
> > + skb_push(skb, IFH_LEN * sizeof(u32));
> > + memcpy(skb->data, ifh, IFH_LEN * sizeof(u32));
> > + skb_put(skb, 4);
> > +
> > + dma_addr = dma_map_single(lan966x->dev, skb->data, skb->len,
> > + DMA_TO_DEVICE);
> > + if (dma_mapping_error(lan966x->dev, dma_addr)) {
> > + dev->stats.tx_dropped++;
> > + err = NETDEV_TX_OK;
> > + goto release;
> > + }
> > +
> > + /* Setup next dcb */
> > + next_dcb = &tx->dcbs[next_to_use];
> > + next_dcb->nextptr = FDMA_DCB_INVALID_DATA;
> > +
> > + next_db = &next_dcb->db[0];
> > + next_db->dataptr = dma_addr;
> > + next_db->status = FDMA_DCB_STATUS_SOF |
> > + FDMA_DCB_STATUS_EOF |
> > + FDMA_DCB_STATUS_INTR |
> > + FDMA_DCB_STATUS_BLOCKO(0) |
> > + FDMA_DCB_STATUS_BLOCKL(skb->len);
> > +
> > + /* Fill up the buffer */
> > + next_dcb_buf = &tx->dcbs_buf[next_to_use];
> > + next_dcb_buf->skb = skb;
> > + next_dcb_buf->dma_addr = dma_addr;
> > + next_dcb_buf->used = true;
> > + next_dcb_buf->ptp = false;
> > +
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > + LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > + next_dcb_buf->ptp = true;
> > +
> > + skb_tx_timestamp(skb);
> > + if (likely(lan966x->tx.activated)) {
> > + /* Connect current dcb to the next db */
> > + dcb = &tx->dcbs[tx->last_in_use];
> > + dcb->nextptr = tx->dma + (next_to_use *
> > + sizeof(struct lan966x_tx_dcb));
> > +
> > + lan966x_fdma_tx_reload(tx);
> > + } else {
> > + /* Because it is first time, then just activate */
> > + lan966x->tx.activated = true;
> > + lan966x_fdma_tx_activate(tx);
> > + }
> > +
> > + /* Move to next dcb because this last in use */
> > + tx->last_in_use = next_to_use;
> > +
> > + dev->stats.tx_packets++;
> > + dev->stats.tx_bytes += skb->len;
>
> I think it's best practice to increase the stats when processing
> completions, you're not sure at this point whether the skb will
> actually get transmitted. Also normally skb could be freed by the
> IRQ handler as soon as its given to HW, but I think you have a lock
> so no risk of UAF on the skb->len access?
Yes, that is correct. I will update this in the IRQ handler.
>
> > + return NETDEV_TX_OK;
> > +
> > +out:
> > + return err;
> > +
> > +release:
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > + LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > + lan966x_ptp_txtstamp_release(port, skb);
> > +
> > + dev_kfree_skb_any(skb);
> > + return err;
>
> I think you're better off returning BUSY directly then you can forgo
> setting err and always return TX_OK.
>
> > +void lan966x_fdma_init(struct lan966x *lan966x)
> > +{
> > + lan966x->rx.lan966x = lan966x;
> > + lan966x->rx.channel_id = FDMA_XTR_CHANNEL;
> > + lan966x->tx.lan966x = lan966x;
> > + lan966x->tx.channel_id = FDMA_INJ_CHANNEL;
> > + lan966x->tx.last_in_use = -1;
> > +
> > + lan966x_fdma_rx_alloc(&lan966x->rx);
> > + lan966x_fdma_tx_alloc(&lan966x->tx);
> > +
> > + lan966x_fdma_rx_start(&lan966x->rx);
>
> Not checking for any errors here is highly suspicious
--
/Horatiu
Powered by blists - more mailing lists