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: <8dbb4087-db01-fbbf-4e96-a5b0e170249a@wanadoo.fr> Date: Mon, 22 May 2023 19:13:43 +0200 From: Christophe JAILLET <christophe.jaillet@...adoo.fr> To: Simon Horman <simon.horman@...igine.com>, Dan Carpenter <dan.carpenter@...aro.org> Cc: Rain River <rain.1986.08.12@...il.com>, Zhu Yanjun <zyjzyj2000@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Ayaz Abdulla <aabdulla@...dia.com>, linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org, netdev@...r.kernel.org Subject: Re: [PATCH net] forcedeth: Fix an error handling path in nv_probe() Le 22/05/2023 à 13:10, Simon Horman a écrit : > On Mon, May 22, 2023 at 01:35:38PM +0300, Dan Carpenter wrote: >> On Mon, May 22, 2023 at 12:12:48PM +0200, Simon Horman wrote: >>> On Sat, May 20, 2023 at 10:30:17AM +0200, Christophe JAILLET wrote: >>>> If an error occures after calling nv_mgmt_acquire_sema(), it should be >>>> undone with a corresponding nv_mgmt_release_sema() call. >>> >>> nit: s/occures/occurs/ >>> >>>> >>>> Add it in the error handling path of the probe as already done in the >>>> remove function. >>> >>> I was going to ask what happens if nv_mgmt_acquire_sema() fails. >>> Then I realised that it always returns 0. >>> >>> Perhaps it would be worth changing it's return type to void at some point. >>> >> >> What? No? It returns true on success and false on failure. >> >> drivers/net/ethernet/nvidia/forcedeth.c >> 5377 static int nv_mgmt_acquire_sema(struct net_device *dev) >> 5378 { >> 5379 struct fe_priv *np = netdev_priv(dev); >> 5380 u8 __iomem *base = get_hwbase(dev); >> 5381 int i; >> 5382 u32 tx_ctrl, mgmt_sema; >> 5383 >> 5384 for (i = 0; i < 10; i++) { >> 5385 mgmt_sema = readl(base + NvRegTransmitterControl) & NVREG_XMITCTL_MGMT_SEMA_MASK; >> 5386 if (mgmt_sema == NVREG_XMITCTL_MGMT_SEMA_FREE) >> 5387 break; >> 5388 msleep(500); >> 5389 } >> 5390 >> 5391 if (mgmt_sema != NVREG_XMITCTL_MGMT_SEMA_FREE) >> 5392 return 0; >> 5393 >> 5394 for (i = 0; i < 2; i++) { >> 5395 tx_ctrl = readl(base + NvRegTransmitterControl); >> 5396 tx_ctrl |= NVREG_XMITCTL_HOST_SEMA_ACQ; >> 5397 writel(tx_ctrl, base + NvRegTransmitterControl); >> 5398 >> 5399 /* verify that semaphore was acquired */ >> 5400 tx_ctrl = readl(base + NvRegTransmitterControl); >> 5401 if (((tx_ctrl & NVREG_XMITCTL_HOST_SEMA_MASK) == NVREG_XMITCTL_HOST_SEMA_ACQ) && >> 5402 ((tx_ctrl & NVREG_XMITCTL_MGMT_SEMA_MASK) == NVREG_XMITCTL_MGMT_SEMA_FREE)) { >> 5403 np->mgmt_sema = 1; >> 5404 return 1; >> ^^^^^^^^^ >> Success path. >> >> 5405 } else >> 5406 udelay(50); >> 5407 } >> 5408 >> 5409 return 0; >> 5410 } > > Thanks Dan, > > my eyes deceived me. > > In that case, my question is: what if nv_mgmt_acquire_sema() fails? > But I think the answer is that nv_mgmt_release_sema() will do > nothing because mgmt_sema is not set. At least, it is my understanding. Can you fix the typo s/occures/occurs/ when applying the patch, or do you really need a v2 only for that? CJ. > > So I think we are good. > > >
Powered by blists - more mailing lists