Messages in this thread | | | Date | Mon, 17 Apr 2023 19:17:34 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next v2] net/ipv6: silence 'passing zero to ERR_PTR()' warning |
| |
On Fri, 14 Apr 2023 09:32:51 +0300 Dan Carpenter wrote: > Also it can return NULL. > > net/xfrm/xfrm_policy.c > 3229 dst = dst_orig; > 3230 } > 3231 ok: > 3232 xfrm_pols_put(pols, drop_pols); > 3233 if (dst && dst->xfrm && > ^^^ > "dst" is NULL.
Don't take my word for it, but AFAICT it's impossible to get there with dst == NULL. I think we can remove this check instead if that's what makes smatch infer that dst may be NULL.
> 3234 dst->xfrm->props.mode == XFRM_MODE_TUNNEL) > 3235 dst->flags |= DST_XFRM_TUNNEL; > 3236 return dst; > ^^^^^^^^^^^ > 3237 > > So in the original code what happened here was: > > net/ipv6/icmp.c > 395 dst2 = xfrm_lookup(net, dst2, flowi6_to_flowi(&fl2), sk, XFRM_LOOKUP_ICMP); > 396 if (!IS_ERR(dst2)) { > > xfrm_lookup() returns NULL. NULL is not an error pointer. > > 397 dst_release(dst); > 398 dst = dst2; > > We set "dst" to NULL. > > 399 } else { > 400 err = PTR_ERR(dst2); > 401 if (err == -EPERM) { > 402 dst_release(dst); > 403 return dst2; > 404 } else > 405 goto relookup_failed; > 406 } > 407 > 408 relookup_failed: > 409 if (dst) > 410 return dst; > > dst is not NULL so we don't return it. > > 411 return ERR_PTR(err); > > However "err" is not set so we do return NULL and Smatch complains about > that. > > Returning ERR_PTR(0); is not necessarily a bug, however 80% of the time > in newly introduced code it is a bug. Here, returning NULL is correct. > So this is a false positive, but the code is just wibbly winding and so > difficult to read. > > 412 }
| |