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

From: Michal Kubiak
Date: Thu Mar 13 2025 - 09:48:38 EST


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>