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@linaro.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

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@linaro.org>

Applied, thanks a lot Dan!

On 3/12/25 6:21 PM, 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@linaro.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.

I think it's better if this patch goes first into the ipsec/xfrm tree,
so that hopefully it gets some serious testing before landing into net-next.

@Steffen, @Herber: could you please take this in your tree?

Thanks,

Paolo

On Wed, Mar 19, 2025 at 06:38:49PM +0100, Paolo Abeni wrote:
> On 3/12/25 6:21 PM, 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@linaro.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.
> 
> I think it's better if this patch goes first into the ipsec/xfrm tree,
> so that hopefully it gets some serious testing before landing into net-next.
> 
> @Steffen, @Herber: could you please take this in your tree?

It is currently sitting in my testing branch and will be merged
to the ipsec tree by the end of the week if no issues were found.

On Wed, Mar 19, 2025 at 06:38:49PM +0100, Paolo Abeni wrote:
> On 3/12/25 6:21 PM, 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@linaro.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.
> 
> I think it's better if this patch goes first into the ipsec/xfrm tree,
> so that hopefully it gets some serious testing before landing into net-next.
> 
> @Steffen, @Herber: could you please take this in your tree?
> 

Thanks.  The issue for me is that I've had a check which complains that
the caller doesn't check for NULL but I've expanded the check and now
there are 16 new warnings.

regards,
dan carpenter

net/mpls/af_mpls.c:633 inet6_fib_lookup_dev() warn: 'dst' can also be NULL
net/xfrm/xfrm_policy.c:2926 xfrm_policy_queue_process() warn: 'dst' can also be NULL
net/xfrm/xfrm_interface_core.c:463 xfrmi_xmit2() warn: 'dst' can also be NULL
net/core/lwt_bpf.c:242 bpf_lwt_xmit_reroute() warn: 'dst' can also be NULL
net/l2tp/l2tp_ip6.c:673 l2tp_ip6_sendmsg() warn: 'dst' can also be NULL
net/l2tp/l2tp_ip6.c:673 l2tp_ip6_sendmsg() warn: 'dst' can also be NULL
net/dccp/ipv6.c:949 dccp_v6_connect() warn: 'dst' can also be NULL
net/ipv4/ip_vti.c:223 vti_xmit() warn: 'dst' can also be NULL
net/ipv6/syncookies.c:249 cookie_v6_check() warn: 'dst' can also be NULL
net/ipv6/syncookies.c:259 cookie_v6_check() warn: 'dst' can also be NULL
net/ipv6/ip6_udp_tunnel.c:171 udp_tunnel6_dst_lookup() warn: 'dst' can also be NULL
net/ipv6/ip6_tunnel.c:1182 ip6_tnl_xmit() warn: 'dst' can also be NULL
net/ipv6/af_inet6.c:859 inet6_sk_rebuild_header() warn: 'dst' can also be NULL
net/ipv6/ip6_vti.c:489 vti6_xmit() warn: 'dst' can also be NULL
net/ipv6/ila/ila_lwt.c:92 ila_output() warn: 'dst' can also be NULL
net/ipv6/ndisc.c:510 ndisc_send_skb() warn: 'dst' can also be NULL
net/ipv6/inet6_connection_sock.c:109 inet6_csk_route_socket() warn: 'dst' can also be NULL
net/ipv6/tcp_ipv6.c:283 tcp_v6_connect() warn: 'dst' can also be NULL
net/ipv6/netfilter/nf_reject_ipv6.c:316 nf_send_reset6() warn: 'dst' can also be NULL
net/ipv6/raw.c:929 rawv6_sendmsg() warn: 'dst' can also be NULL
net/ipv6/raw.c:929 rawv6_sendmsg() warn: 'dst' can also be NULL
net/ipv6/datagram.c:114 ip6_datagram_dst_update() warn: 'dst' can also be NULL
drivers/net/vrf.c:447 vrf_process_v6_outbound() warn: 'dst' can also be NULL
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:1156 chtls_recv_sock() warn: 'dst' can also be NULL
drivers/net/ethernet/netronome/nfp/flower/action.c:475 nfp_fl_set_tun() warn: 'dst' can also be NULL
drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c:826 nfp_tunnel_request_route_v6() warn: 'dst' can also be NULL
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c:464 mlx5e_route_lookup_ipv6_get() warn: 'dst' can also be NULL
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c:466 mlx5e_route_lookup_ipv6_get() warn: 'dst' can also be NULL
drivers/net/wireguard/socket.c:153 send6() warn: 'dst' can also be NULL
drivers/infiniband/core/addr.c:434 addr6_resolve() warn: 'dst' can also be NULL

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@linaro.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@intel.com>