Smatch complains that if xfrm_lookup() returns NULL then this does a
weird thing with "err":
net/ ipv6/ icmp.c:411 icmpv6_route_lookup()
warn: passing zero to ERR_PTR()
Merge conditional paths and remove unnecessary 'else'. No functional
change.
Signed-off-by: Haoyi Liu <[email protected]>
Reviewed-by: Dongliang Mu <[email protected]>
---
v1->v2: Remove unnecessary 'else'.
The issue is found by static analysis, and the patch is remains untested.
---
net/ipv6/icmp.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 1f53f2a74480..a12eef11c7ee 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -393,17 +393,12 @@ static struct dst_entry *icmpv6_route_lookup(struct net *net,
goto relookup_failed;
dst2 = xfrm_lookup(net, dst2, flowi6_to_flowi(&fl2), sk, XFRM_LOOKUP_ICMP);
- if (!IS_ERR(dst2)) {
- dst_release(dst);
- dst = dst2;
- } else {
- err = PTR_ERR(dst2);
- if (err == -EPERM) {
- dst_release(dst);
- return dst2;
- } else
- goto relookup_failed;
- }
+ err = PTR_ERR_OR_ZERO(dst2);
+ if (err && err != -EPERM)
+ goto relookup_failed;
+
+ dst_release(dst);
+ return dst2; /* returns success or ERR_PTR(-EPERM) */
relookup_failed:
if (dst)
--
2.40.0
On 4/13/2023 3:10 AM, Haoyi Liu wrote:
> Smatch complains that if xfrm_lookup() returns NULL then this does a
> weird thing with "err":
>
> net/ ipv6/ icmp.c:411 icmpv6_route_lookup()
> warn: passing zero to ERR_PTR()
>
> Merge conditional paths and remove unnecessary 'else'. No functional
> change.
>
> Signed-off-by: Haoyi Liu <[email protected]>
> Reviewed-by: Dongliang Mu <[email protected]>
> ---
> v1->v2: Remove unnecessary 'else'.
> The issue is found by static analysis, and the patch is remains untested.
> ---
> net/ipv6/icmp.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index 1f53f2a74480..a12eef11c7ee 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -393,17 +393,12 @@ static struct dst_entry *icmpv6_route_lookup(struct net *net,
> goto relookup_failed;
>
> dst2 = xfrm_lookup(net, dst2, flowi6_to_flowi(&fl2), sk, XFRM_LOOKUP_ICMP);
> - if (!IS_ERR(dst2)) {
> - dst_release(dst);
> - dst = dst2;
> - } else {
> - err = PTR_ERR(dst2);
> - if (err == -EPERM) {
> - dst_release(dst);
> - return dst2;
> - } else
> - goto relookup_failed;
> - }
> + err = PTR_ERR_OR_ZERO(dst2);
> + if (err && err != -EPERM)
> + goto relookup_failed;
> +
> + dst_release(dst);
> + return dst2; /* returns success or ERR_PTR(-EPERM) */
>
This result looks much cleaner!
Reviewed-by: Jacob Keller <[email protected]>
Thanks,
Jake
> relookup_failed:
> if (dst)
On 4/13/23 4:10 AM, Haoyi Liu wrote:
> Smatch complains that if xfrm_lookup() returns NULL then this does a
> weird thing with "err":
xfrm_lookup is a wrapper around xfrm_lookup_with_ifid which returns
either either a valid dst or ERR_PTR(err).
On Thu, Apr 13, 2023 at 06:32:24PM -0600, David Ahern wrote:
> On 4/13/23 4:10 AM, Haoyi Liu wrote:
> > Smatch complains that if xfrm_lookup() returns NULL then this does a
> > weird thing with "err":
>
> xfrm_lookup is a wrapper around xfrm_lookup_with_ifid which returns
> either either a valid dst or ERR_PTR(err).
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.
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 }
regards,
dan carpenter
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 }
On 4/17/23 8:17 PM, Jakub Kicinski wrote:
> 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.
>
That was my conclusion as well staring at it multiple times, but given
the horrible maze of goto's I cannot definitively say that.
> -----原始邮件-----
> 发件人: "Haoyi Liu" <[email protected]>
> 发送时间: 2023-04-13 18:10:05 (星期四)
> 收件人: "David S. Miller" <[email protected]>, "David Ahern" <[email protected]>, "Eric Dumazet" <[email protected]>, "Jakub Kicinski" <[email protected]>, "Paolo Abeni" <[email protected]>
> 抄送: [email protected], [email protected], [email protected], "Haoyi Liu" <[email protected]>, "Dongliang Mu" <[email protected]>, [email protected], [email protected]
> 主题: [PATCH net-next v2] net/ipv6: silence 'passing zero to ERR_PTR()' warning
>
> Smatch complains that if xfrm_lookup() returns NULL then this does a
> weird thing with "err":
>
> net/ ipv6/ icmp.c:411 icmpv6_route_lookup()
> warn: passing zero to ERR_PTR()
>
> Merge conditional paths and remove unnecessary 'else'. No functional
> change.
>
> Signed-off-by: Haoyi Liu <[email protected]>
> Reviewed-by: Dongliang Mu <[email protected]>
> ---
> v1->v2: Remove unnecessary 'else'.
> The issue is found by static analysis, and the patch is remains untested.
> ---
> net/ipv6/icmp.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index 1f53f2a74480..a12eef11c7ee 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -393,17 +393,12 @@ static struct dst_entry *icmpv6_route_lookup(struct net *net,
> goto relookup_failed;
>
> dst2 = xfrm_lookup(net, dst2, flowi6_to_flowi(&fl2), sk, XFRM_LOOKUP_ICMP);
> - if (!IS_ERR(dst2)) {
> - dst_release(dst);
> - dst = dst2;
> - } else {
> - err = PTR_ERR(dst2);
> - if (err == -EPERM) {
> - dst_release(dst);
> - return dst2;
> - } else
> - goto relookup_failed;
> - }
> + err = PTR_ERR_OR_ZERO(dst2);
> + if (err && err != -EPERM)
> + goto relookup_failed;
> +
> + dst_release(dst);
> + return dst2; /* returns success or ERR_PTR(-EPERM) */
>
> relookup_failed:
> if (dst)
> --
> 2.40.0
ping?
On Thu, 11 May 2023 22:52:25 +0800 (GMT+08:00) 刘浩毅 wrote:
> ping?
Do not send "pings", if you don't understand the discussion reply
to the correct email and say "I don't understand".