* [bug report] net/funeth: probing and netdev ops
@ 2022-03-04 13:17 Dan Carpenter
2022-03-04 18:40 ` Dimitris Michailidis
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-03-04 13:17 UTC (permalink / raw)
To: d.michailidis; +Cc: kernel-janitors
Hello Dimitris Michailidis,
The patch ee6373ddf3a9: "net/funeth: probing and netdev ops" from Feb
24, 2022, leads to the following Smatch static checker warning:
drivers/net/ethernet/fungible/funeth/funeth_main.c:477 fun_free_rings()
warn: 'rxqs' was already freed.
drivers/net/ethernet/fungible/funeth/funeth_main.c
443 static void fun_free_rings(struct net_device *netdev, struct fun_qset *qset)
444 {
445 struct funeth_priv *fp = netdev_priv(netdev);
446 struct funeth_txq **xdpqs = qset->xdpqs;
447 struct funeth_rxq **rxqs = qset->rxqs;
448
449 /* qset may not specify any queues to operate on. In that case the
450 * currently installed queues are implied.
451 */
452 if (!rxqs) {
453 rxqs = rtnl_dereference(fp->rxqs);
454 xdpqs = rtnl_dereference(fp->xdpqs);
455 qset->txqs = fp->txqs;
456 qset->nrxqs = netdev->real_num_rx_queues;
457 qset->ntxqs = netdev->real_num_tx_queues;
458 qset->nxdpqs = fp->num_xdpqs;
459 }
460 if (!rxqs)
461 return;
462
463 if (rxqs == rtnl_dereference(fp->rxqs)) {
464 rcu_assign_pointer(fp->rxqs, NULL);
465 rcu_assign_pointer(fp->xdpqs, NULL);
466 synchronize_net();
467 fp->txqs = NULL;
468 }
469
470 free_rxqs(rxqs, qset->nrxqs, qset->rxq_start, qset->state);
471 free_txqs(qset->txqs, qset->ntxqs, qset->txq_start, qset->state);
472 free_xdpqs(xdpqs, qset->nxdpqs, qset->xdpq_start, qset->state);
473 if (qset->state == FUN_QSTATE_DESTROYED)
474 kfree(rxqs);
^^^^^^^^^^^
Should this return or set "rxqs = NULL" or something?
475
476 /* Tell the caller which queues were operated on. */
--> 477 qset->rxqs = rxqs;
^^^^^^^^^^^^^^^^^^
Only bad things will happen with safing this freed pointer.
478 qset->xdpqs = xdpqs;
479 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] net/funeth: probing and netdev ops
2022-03-04 13:17 [bug report] net/funeth: probing and netdev ops Dan Carpenter
@ 2022-03-04 18:40 ` Dimitris Michailidis
0 siblings, 0 replies; 6+ messages in thread
From: Dimitris Michailidis @ 2022-03-04 18:40 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
On Fri, Mar 4, 2022 at 5:17 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Dimitris Michailidis,
>
> The patch ee6373ddf3a9: "net/funeth: probing and netdev ops" from Feb
> 24, 2022, leads to the following Smatch static checker warning:
>
> drivers/net/ethernet/fungible/funeth/funeth_main.c:477 fun_free_rings()
> warn: 'rxqs' was already freed.
>
> drivers/net/ethernet/fungible/funeth/funeth_main.c
> 443 static void fun_free_rings(struct net_device *netdev, struct fun_qset *qset)
> 444 {
> 445 struct funeth_priv *fp = netdev_priv(netdev);
> 446 struct funeth_txq **xdpqs = qset->xdpqs;
> 447 struct funeth_rxq **rxqs = qset->rxqs;
> 448
> 449 /* qset may not specify any queues to operate on. In that case the
> 450 * currently installed queues are implied.
> 451 */
> 452 if (!rxqs) {
> 453 rxqs = rtnl_dereference(fp->rxqs);
> 454 xdpqs = rtnl_dereference(fp->xdpqs);
> 455 qset->txqs = fp->txqs;
> 456 qset->nrxqs = netdev->real_num_rx_queues;
> 457 qset->ntxqs = netdev->real_num_tx_queues;
> 458 qset->nxdpqs = fp->num_xdpqs;
> 459 }
> 460 if (!rxqs)
> 461 return;
> 462
> 463 if (rxqs == rtnl_dereference(fp->rxqs)) {
> 464 rcu_assign_pointer(fp->rxqs, NULL);
> 465 rcu_assign_pointer(fp->xdpqs, NULL);
> 466 synchronize_net();
> 467 fp->txqs = NULL;
> 468 }
> 469
> 470 free_rxqs(rxqs, qset->nrxqs, qset->rxq_start, qset->state);
> 471 free_txqs(qset->txqs, qset->ntxqs, qset->txq_start, qset->state);
> 472 free_xdpqs(xdpqs, qset->nxdpqs, qset->xdpq_start, qset->state);
> 473 if (qset->state == FUN_QSTATE_DESTROYED)
> 474 kfree(rxqs);
> ^^^^^^^^^^^
> Should this return or set "rxqs = NULL" or something?
>
> 475
> 476 /* Tell the caller which queues were operated on. */
> --> 477 qset->rxqs = rxqs;
> ^^^^^^^^^^^^^^^^^^
> Only bad things will happen with safing this freed pointer.
There is a legitimate use case of passing this value back, namely using the
pointer value itself to identify the queue set that was operated on.
One must not
dereference the pointer of course. The same is true for other fields here, e.g.,
xdpqs on the next line may also be pointing to freed memory. Note also that the
caller may initialize this field to the same value so it isn't the
case that if line 477
were removed the field could not be pointing to freed memory.
Having said that there isn't a current caller that uses these pointers
even as just
values if they've asked for memory to be freed. I don't think there's
a bug here though.
>
> 478 qset->xdpqs = xdpqs;
> 479 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug report] net/funeth: probing and netdev ops
@ 2024-08-15 11:29 Dan Carpenter
2024-08-15 15:59 ` Jakub Kicinski
2024-08-16 9:36 ` Dimitris Michailidis
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2024-08-15 11:29 UTC (permalink / raw)
To: Dimitris Michailidis; +Cc: netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] net/funeth: probing and netdev ops
2024-08-15 11:29 Dan Carpenter
@ 2024-08-15 15:59 ` Jakub Kicinski
2024-08-16 9:36 ` Dimitris Michailidis
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-08-15 15:59 UTC (permalink / raw)
To: Dimitris Michailidis; +Cc: Dan Carpenter, netdev
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".
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] net/funeth: probing and netdev ops
2024-08-15 11:29 Dan Carpenter
2024-08-15 15:59 ` Jakub Kicinski
@ 2024-08-16 9:36 ` Dimitris Michailidis
1 sibling, 0 replies; 6+ messages in thread
From: Dimitris Michailidis @ 2024-08-16 9:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: netdev@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug report] net/funeth: probing and netdev ops
@ 2025-03-11 15:01 Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2025-03-11 15:01 UTC (permalink / raw)
To: Dimitris Michailidis; +Cc: netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-11 15:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-04 13:17 [bug report] net/funeth: probing and netdev ops Dan Carpenter
2022-03-04 18:40 ` Dimitris Michailidis
-- strict thread matches above, loose matches on Subject: below --
2024-08-15 11:29 Dan Carpenter
2024-08-15 15:59 ` Jakub Kicinski
2024-08-16 9:36 ` Dimitris Michailidis
2025-03-11 15:01 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.