Re: [PATCH net-next] xfrm: Remove unnecessary NULL check in xfrm_lookup_with_ifid()

[Michal Kubiak]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxx>