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>
