linux-kernel - Re: [PATCH net-next] net/rose: Remove unnecessary if check in rose_dev

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] [day] [month] [year] [list]

Message-ID: <96fbe379-cf8e-44e9-aeaf-a8beee2eda9c@suswa.mountain>
Date: Mon, 14 Jul 2025 21:03:53 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Ingo Molnar <mingo@...nel.org>,
	Kohei Enju <enjuk@...zon.com>, Thomas Gleixner <tglx@...utronix.de>,
	linux-hams@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net/rose: Remove unnecessary if check in
 rose_dev_first()

On Fri, Jul 04, 2025 at 10:33:08AM +0200, Thorsten Blum wrote:
> dev_hold() already checks if its argument is NULL.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> ---
>  net/rose/rose_route.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
> index b72bf8a08d48..35e21a2bec9c 100644
> --- a/net/rose/rose_route.c
> +++ b/net/rose/rose_route.c
> @@ -608,8 +608,7 @@ struct net_device *rose_dev_first(void)
>  			if (first == NULL || strncmp(dev->name, first->name, 3) < 0)
>  				first = dev;
>  	}
> -	if (first)
> -		dev_hold(first);
> +	dev_hold(first);

I'm not a fan of these sorts of "remove the NULL check" patches in
general.  Sure it removes a line of code, but does it really improve
readability?  I feel like someone reading this code might think a NULL
check was required.

I guess there is also an argument that this is a tiny speedup.  That
could be a valid argument especially if we had benchmarking data to back
it up.

Of course, if you're planning to take over this code and be the
maintainer of it, then you get to do whatever you feel is best.  So if
this change were part of a larger change where you were taking over then
that's fine.

regards,
dan carpenter