netdev - Re: [PATCH net-next] xfrm: Remove unnecessary NULL check in xfrm_lookup_with

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: <Z9LiE45pxSXFJM9H@localhost.localdomain>
Date: Thu, 13 Mar 2025 14:48:03 +0100
From: Michal Kubiak <michal.kubiak@...el.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: Steffen Klassert <steffen.klassert@...unet.com>, Herbert Xu
	<herbert@...dor.apana.org.au>, "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>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH net-next] xfrm: Remove unnecessary NULL check in
 xfrm_lookup_with_ifid()

On Wed, Mar 12, 2025 at 08:21:13PM +0300, Dan Carpenter wrote:
> This NULL check is unnecessary and can be removed.  It confuses
> Smatch static analysis tool because it makes Smatch think that
> xfrm_lookup_with_ifid() can return a mix of NULL pointers and errors so
> it creates a lot of false positives.  Remove it.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> ---
> I have wanted to remove this NULL check for a long time.  Someone
> said it could be done safely.  But please, please, review this
> carefully.
> 
>  net/xfrm/xfrm_policy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 6551e588fe52..30970d40a454 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3294,7 +3294,7 @@ struct dst_entry *xfrm_lookup_with_ifid(struct net *net,
>  
>  ok:
>  	xfrm_pols_put(pols, drop_pols);
> -	if (dst && dst->xfrm &&
> +	if (dst->xfrm &&
>  	    (dst->xfrm->props.mode == XFRM_MODE_TUNNEL ||
>  	     dst->xfrm->props.mode == XFRM_MODE_IPTFS))
>  		dst->flags |= DST_XFRM_TUNNEL;
> -- 
> 2.47.2
> 
> 


After analyzing the function, I haven't found any flow where 'dst' can be
NULL. So, it seems the NULL-ptr check can be safely removed.

Even after jumping directly to 'no_transform', 'num_xfrms' should be
less than or equal to zero. In the first case we'll go to 'error' and in
the second case 'dst_orig' will be assigned to 'dst'.
The only doubt I have is about 'dst_orig' itself, but the function seems
to assume that the 'dst_orig' parameter is never NULL because it is
dereferenced at the beginning of the function:

	u16 family = dst_orig->ops->family;

So, it shouldn't be a problem in this case.

(Probably some feedback from someone who has an experience with this code
would be more beneficial).


Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>