netdev - Re: [PATCH net-next 2/7] bnxt_en: Remove a redundant NULL check in bnxt_register

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <af839ea4-1a41-4250-b5d2-61db182d26bf@intel.com>
Date: Tue, 9 Apr 2024 16:35:34 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Michael Chan <michael.chan@...adcom.com>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <pavan.chebbi@...adcom.com>,
	<andrew.gospodarek@...adcom.com>, Kalesh AP
	<kalesh-anakkur.purayil@...adcom.com>, Vikas Gupta
	<vikas.gupta@...adcom.com>, Somnath Kotur <somnath.kotur@...adcom.com>
Subject: Re: [PATCH net-next 2/7] bnxt_en: Remove a redundant NULL check in
 bnxt_register_dev()



On 4/9/2024 2:54 PM, Michael Chan wrote:
> From: Kalesh AP <kalesh-anakkur.purayil@...adcom.com>
> 
> The memory for "edev->ulp_tbl" is allocated inside the
> bnxt_rdma_aux_device_init() function. If it fails, the driver
> will not create the auxiliary device for RoCE. Hence the NULL
> check inside bnxt_register_dev() is unnecessary.
> 
> Reviewed-by: Vikas Gupta <vikas.gupta@...adcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@...adcom.com>
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@...adcom.com>
> Signed-off-by: Michael Chan <michael.chan@...adcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index 86dcd2c76587..fd890819d4bc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> @@ -64,9 +64,6 @@ int bnxt_register_dev(struct bnxt_en_dev *edev,
>  		return -ENOMEM;
>  
>  	ulp = edev->ulp_tbl;
> -	if (!ulp)
> -		return -ENOMEM;
> -
>  	ulp->handle = handle;
>  	rcu_assign_pointer(ulp->ulp_ops, ulp_ops);
>  

Ok, so ulp_tbl is setup by bnxt_rdma_aux_device_init, and you're
claiming that the only way bnxt_register_dev gets called is if the
auxiliary driver loads?

But bnxt_register_dev is a simple direct-exported function, so the bnxt
driver itself doesn't enforce this. However, if we go look at the
infiniband driver that does call bnxt_register_dev, it only calls this
as part of its auxiliary device probe.

This does make the NULL check here seem redundant, though I think its a
lot more layered than a normal removal of a NULL check since this
crosses module boundary and its a bit subtle to follow the full flow of
the auxiliary drivers.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>