[bug report] net/funeth: probing and netdev ops

[bug report] net/funeth: probing and netdev ops

[Dan Carpenter]

Hello Dimitris Michailidis,

Commit ee6373ddf3a9 ("net/funeth: probing and netdev ops") from Feb
24, 2022 (linux-next), leads to the following Smatch static checker
warning:

	drivers/net/ethernet/fungible/funeth/funeth_main.c:475 fun_free_rings()
	warn: 'rxqs' was already freed. (line 472)

drivers/net/ethernet/fungible/funeth/funeth_main.c
    441 static void fun_free_rings(struct net_device *netdev, struct fun_qset *qset)
    442 {
    443         struct funeth_priv *fp = netdev_priv(netdev);
    444         struct funeth_txq **xdpqs = qset->xdpqs;
    445         struct funeth_rxq **rxqs = qset->rxqs;
    446 
    447         /* qset may not specify any queues to operate on. In that case the
    448          * currently installed queues are implied.
    449          */
    450         if (!rxqs) {
    451                 rxqs = rtnl_dereference(fp->rxqs);
    452                 xdpqs = rtnl_dereference(fp->xdpqs);
    453                 qset->txqs = fp->txqs;
    454                 qset->nrxqs = netdev->real_num_rx_queues;
    455                 qset->ntxqs = netdev->real_num_tx_queues;
    456                 qset->nxdpqs = fp->num_xdpqs;
    457         }
    458         if (!rxqs)
    459                 return;
    460 
    461         if (rxqs == rtnl_dereference(fp->rxqs)) {
    462                 rcu_assign_pointer(fp->rxqs, NULL);
    463                 rcu_assign_pointer(fp->xdpqs, NULL);
    464                 synchronize_net();
    465                 fp->txqs = NULL;
    466         }
    467 
    468         free_rxqs(rxqs, qset->nrxqs, qset->rxq_start, qset->state);
    469         free_txqs(qset->txqs, qset->ntxqs, qset->txq_start, qset->state);
    470         free_xdpqs(xdpqs, qset->nxdpqs, qset->xdpq_start, qset->state);
    471         if (qset->state == FUN_QSTATE_DESTROYED)
    472                 kfree(rxqs);
                        ^^^^^^^^^^^
Freed.

    473 
    474         /* Tell the caller which queues were operated on. */
--> 475         qset->rxqs = rxqs;
                             ^^^^^
why are we saving a freed pointer?

    476         qset->xdpqs = xdpqs;
    477 }

regards,
dan carpenter

Re: [bug report] net/funeth: probing and netdev ops

[Jakub Kicinski]

On Thu, 15 Aug 2024 14:29:38 +0300 Dan Carpenter wrote:
> Hello Dimitris Michailidis,
> 
> Commit ee6373ddf3a9 ("net/funeth: probing and netdev ops") from Feb
> 24, 2022 (linux-next), leads to the following Smatch static checker
> warning:
> 
> 	drivers/net/ethernet/fungible/funeth/funeth_main.c:475 fun_free_rings()
> 	warn: 'rxqs' was already freed. (line 472)

Somewhat related, Dimitris is Fungible still active?
Advanced NIC startup are very close to my heart but I wonder if anyone
is actually using these in the wild, given the "exit".

Re: [bug report] net/funeth: probing and netdev ops

[Dimitris Michailidis]

On Thu, 15 Aug 2024 14:29:38 +0300 Dan Carpenter wrote:
> Hello Dimitris Michailidis,
>
> Commit ee6373ddf3a9 ("net/funeth: probing and netdev ops") from Feb
> 24, 2022 (linux-next), leads to the following Smatch static checker
> warning:
>
>        drivers/net/ethernet/fungible/funeth/funeth_main.c:475 fun_free_rings()
>        warn: 'rxqs' was already freed. (line 472)
>
> drivers/net/ethernet/fungible/funeth/funeth_main.c
>     441 static void fun_free_rings(struct net_device *netdev, struct fun_qset *qset)
>     442 {
...
>     468         free_rxqs(rxqs, qset->nrxqs, qset->rxq_start, qset->state);
>     469         free_txqs(qset->txqs, qset->ntxqs, qset->txq_start, qset->state);
>     470         free_xdpqs(xdpqs, qset->nxdpqs, qset->xdpq_start, qset->state);
>     471         if (qset->state == FUN_QSTATE_DESTROYED)
>     472                 kfree(rxqs);
>                         ^^^^^^^^^^^
> Freed.
>
>     473
>     474         /* Tell the caller which queues were operated on. */
> --> 475         qset->rxqs = rxqs;
>                              ^^^^^
> why are we saving a freed pointer?

The field may be NULL on entry to the function. The assignment tells the
caller that queues were freed as this function doesn't use some other
success indicator.

Note that if the caller passes a non-NULL value to begin with, this
assignment is no-op and the field still points to freed memory, it
retains the value it had upon entry. So, the assignment doesn't make
the field more dangerous than it can be without the assignment.

Though the value is the address of freed memory, it can still be compared
to other values. It's just illegal to dereference it.

>     476         qset->xdpqs = xdpqs;
>     477 }
>
> regards,
> dan carpenter

[bug report] net/funeth: probing and netdev ops

[Dan Carpenter]

Hello Dimitris Michailidis,

Commit ee6373ddf3a9 ("net/funeth: probing and netdev ops") from Feb
24, 2022 (linux-next), leads to the following Smatch static checker
warning:

	drivers/net/ethernet/fungible/funeth/funeth_main.c:333 fun_alloc_queue_irqs()
	warn: 'irq' can also be NULL

drivers/net/ethernet/fungible/funeth/funeth_main.c
    319 static int fun_alloc_queue_irqs(struct net_device *dev, unsigned int ntx,
    320                                 unsigned int nrx)
    321 {
    322         struct funeth_priv *fp = netdev_priv(dev);
    323         int node = dev_to_node(&fp->pdev->dev);
    324         struct fun_irq *irq;
    325         unsigned int i;
    326 
    327         for (i = fp->num_tx_irqs; i < ntx; i++) {
    328                 irq = fun_alloc_qirq(fp, i, node, 0);
                               ^^^^^^^^^^^^^
The fun_alloc_qirq() function can return NULL.

    329                 if (IS_ERR(irq))
    330                         return PTR_ERR(irq);
    331 
    332                 fp->num_tx_irqs++;
--> 333                 netif_napi_add_tx(dev, &irq->napi, fun_txq_napi_poll);
    334         }
    335 

The problem is this:

   249  static struct fun_irq *fun_alloc_qirq(struct funeth_priv *fp, unsigned int idx,
   250                                        int node, unsigned int xa_idx_offset)
   251  {
   252          struct fun_irq *irq;
   253          int cpu, res;
   254  
   255          cpu = cpumask_local_spread(idx, node);
   256          node = cpu_to_mem(cpu);
   257  
   258          irq = kzalloc_node(sizeof(*irq), GFP_KERNEL, node);
   259          if (!irq)
   260                  return ERR_PTR(-ENOMEM);
   261  
   262          res = fun_reserve_irqs(fp->fdev, 1, &irq->irq_idx);
   263          if (res != 1)
   264                  goto free_irq;

The error code is not set on this path.  This is the only caller.  Why not
modify fun_reserve_irqs() to just return zero on success and negative
failures?  Are we likely to need the current API in the near future?

   265  
   266          res = xa_insert(&fp->irqs, idx + xa_idx_offset, irq, GFP_KERNEL);
   267          if (res)
   268                  goto release_irq;
   269  
   270          irq->irq = pci_irq_vector(fp->pdev, irq->irq_idx);
   271          cpumask_set_cpu(cpu, &irq->affinity_mask);
   272          irq->aff_notify.notify = fun_irq_aff_notify;
   273          irq->aff_notify.release = fun_irq_aff_release;
   274          irq->state = FUN_IRQ_INIT;
   275          return irq;
   276  
   277  release_irq:
   278          fun_release_irqs(fp->fdev, 1, &irq->irq_idx);
   279  free_irq:
   280          kfree(irq);
   281          return ERR_PTR(res);
   282  }

regards,
dan carpenter