Re: [PATCH net-next] xfrm: Remove unnecessary NULL check in xfrm_lookup_with_ifid()
From: Michal Kubiak <hidden>
Date: 2025-03-13 13:48:21
Also in:
kernel-janitors, lkml
On Wed, Mar 12, 2025 at 08:21:13PM +0300, Dan Carpenter wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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 <redacted>