netdev - Re: [PATCH] can: ctucanfd: Remove a useless netif_napi

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 May 2023 11:51:46 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Pavel Pisa <pisa@....felk.cvut.cz>
Cc: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Ondrej Ille <ondrej.ille@...il.com>,
	Wolfgang Grandegger <wg@...ndegger.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	linux-can@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] can: ctucanfd: Remove a useless netif_napi_del() call

On 18.05.2023 09:32:38, Pavel Pisa wrote:
> Dear Christophe,
> 
> On Thursday 18 of May 2023 09:10:39 Christophe JAILLET wrote:
> > free_candev() already calls netif_napi_del(), so there is no need to call
> > it explicitly. It is harmless, but useless.
> >
> > This makes the code mode consistent with the error handling path of
> > ctucan_probe_common().
> 
> OK, but I would suggest to consider to keep sequence in sync with
> 
> linux/drivers/net/can/ctucanfd/ctucanfd_pci.c
> 
> where is netif_napi_del() used as well
> 
>         while ((priv = list_first_entry_or_null(&bdata->ndev_list_head, struct ctucan_priv,
>                                                 peers_on_pdev)) != NULL) {
>                 ndev = priv->can.dev;
> 
>                 unregister_candev(ndev);
> 
>                 netif_napi_del(&priv->napi);
> 
>                 list_del_init(&priv->peers_on_pdev);
>                 free_candev(ndev);
>         }
> 
> On the other hand, if interrupt can be called for device between
> unregister_candev() and free_candev()

At least the case of an "interrupt during ctucan_pci_remove()" is a bug,
as there is no IRQ handler registered. The IRQ handler is registered in
ctucan_open() and freed in ctucan_close().

> or some other callback
> which is prevented by netif_napi_del() now then I would consider
> to keep explicit netif_napi_del() to ensure that no callback
> is activated to driver there.

Napi itself is shut down, too, as there is a call to napi_disable() in
ctucan_close().

> And for PCI integration it is more
> critical because list_del_init(&priv->peers_on_pdev); appears in
> between and I would prefer that no interrupt appears when instance
> is not on the peers list anymore. Even that would not be a problem
> for actual CTU CAN FD implementation, peers are accessed only during
> physical device remove, but I have worked on other controllers
> in past, which required to coordinate with peers in interrupt
> handling...
> 
> So I would be happy for some feedback what is actual guarantee
> when device is stopped.

After a ifup; ifdown;, which corresponds to ctucan_open(),
ctucan_close() in the driver, the device should be shut down, no
interrupts active. You might even power down the device, although things
get a little more complicated with HW timestamping or even PTP enabled.

> May it be that it would be even more robust to run removal
> with two loop where the first one calls unregister_candev()
> and netif_napi_del() and only the second one removes from peers
> and call free_candev()... But I am not sure there and it is not
> problem in actual driver because peers are not used in any
> other place...

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)