On Fri, Mar 4, 2022 at 5:17 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
