Coverity: __sock_gen_cookie(): Error handling issues

Coverity: __sock_gen_cookie(): Error handling issues

[coverity-bot]

Hello!

This is an experimental semi-automated report about issues detected by
Coverity from a scan of next-20221117 as part of the linux-next scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified
lines of code (noted below) that were touched by commits:

  Wed Nov 16 12:42:01 2022 +0000
    4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")

Coverity reported the following:

*** CID 1527347:  Error handling issues  (CHECKED_RETURN)
net/core/sock_diag.c:33 in __sock_gen_cookie()
27     {
28     	u64 res = atomic64_read(&sk->sk_cookie);
29
30     	if (!res) {
31     		u64 new = gen_cookie_next(&sock_cookie);
32
vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
33     		atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
34     	}
35     	return res;
36     }
37
38     int sock_diag_check_cookie(struct sock *sk, const __u32 *cookie)

If this is a false positive, please let us know so we can mark it as
such, or teach the Coverity rules to be smarter. If not, please make
sure fixes get into linux-next. :) For patches fixing this, please
include these lines (but double-check the "Fixes" first):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1527347 ("Error handling issues")
Fixes: 4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")

Thanks for your attention!

-- 
Coverity-bot

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Eric Dumazet]

On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@chromium.org> wrote:
>
> Hello!
>
> This is an experimental semi-automated report about issues detected by
> Coverity from a scan of next-20221117 as part of the linux-next scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
>
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by commits:
>
>   Wed Nov 16 12:42:01 2022 +0000
>     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
>
> Coverity reported the following:
>
> *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> net/core/sock_diag.c:33 in __sock_gen_cookie()
> 27     {
> 28      u64 res = atomic64_read(&sk->sk_cookie);
> 29
> 30      if (!res) {
> 31              u64 new = gen_cookie_next(&sock_cookie);
> 32
> vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);


Hmmm. for some reason I thought @res was always updated...

A fix would be to read sk->sk_cookie, but I guess your tool will still
complain we do not care
of  atomic64_try_cmpxchg() return value ?

diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
                u64 new = gen_cookie_next(&sock_cookie);

                atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
+               /* Another cpu/thread might have won the race,
+                * reload the final value.
+                */
+               res = atomic64_read(&sk->sk_cookie);
        }
        return res;
 }

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Kees Cook]

On Thu, Nov 17, 2022 at 02:49:55PM -0800, Eric Dumazet wrote:
> On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@chromium.org> wrote:
> >
> > Hello!
> >
> > This is an experimental semi-automated report about issues detected by
> > Coverity from a scan of next-20221117 as part of the linux-next scan project:
> > https://scan.coverity.com/projects/linux-next-weekly-scan
> >
> > You're getting this email because you were associated with the identified
> > lines of code (noted below) that were touched by commits:
> >
> >   Wed Nov 16 12:42:01 2022 +0000
> >     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
> >
> > Coverity reported the following:
> >
> > *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > net/core/sock_diag.c:33 in __sock_gen_cookie()
> > 27     {
> > 28      u64 res = atomic64_read(&sk->sk_cookie);
> > 29
> > 30      if (!res) {
> > 31              u64 new = gen_cookie_next(&sock_cookie);
> > 32
> > vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> > 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> 
> 
> Hmmm. for some reason I thought @res was always updated...
> 
> A fix would be to read sk->sk_cookie, but I guess your tool will still
> complain we do not care
> of  atomic64_try_cmpxchg() return value ?
> 
> diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
> 100644
> --- a/net/core/sock_diag.c
> +++ b/net/core/sock_diag.c
> @@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
>                 u64 new = gen_cookie_next(&sock_cookie);
> 
>                 atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> +               /* Another cpu/thread might have won the race,
> +                * reload the final value.
> +                */
> +               res = atomic64_read(&sk->sk_cookie);
>         }
>         return res;
>  }

I think it's saying it was expecting an update loop -- i.e. to make sure
the value actually got swapped (the "try" part...)?

-- 
Kees Cook

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Eric Dumazet]

On Thu, Nov 17, 2022 at 3:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Nov 17, 2022 at 02:49:55PM -0800, Eric Dumazet wrote:
> > On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@chromium.org> wrote:
> > >
> > > Hello!
> > >
> > > This is an experimental semi-automated report about issues detected by
> > > Coverity from a scan of next-20221117 as part of the linux-next scan project:
> > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > >
> > > You're getting this email because you were associated with the identified
> > > lines of code (noted below) that were touched by commits:
> > >
> > >   Wed Nov 16 12:42:01 2022 +0000
> > >     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
> > >
> > > Coverity reported the following:
> > >
> > > *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > net/core/sock_diag.c:33 in __sock_gen_cookie()
> > > 27     {
> > > 28      u64 res = atomic64_read(&sk->sk_cookie);
> > > 29
> > > 30      if (!res) {
> > > 31              u64 new = gen_cookie_next(&sock_cookie);
> > > 32
> > > vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> > > 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> >
> >
> > Hmmm. for some reason I thought @res was always updated...
> >
> > A fix would be to read sk->sk_cookie, but I guess your tool will still
> > complain we do not care
> > of  atomic64_try_cmpxchg() return value ?
> >
> > diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> > index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
> > 100644
> > --- a/net/core/sock_diag.c
> > +++ b/net/core/sock_diag.c
> > @@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
> >                 u64 new = gen_cookie_next(&sock_cookie);
> >
> >                 atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > +               /* Another cpu/thread might have won the race,
> > +                * reload the final value.
> > +                */
> > +               res = atomic64_read(&sk->sk_cookie);
> >         }
> >         return res;
> >  }
>
> I think it's saying it was expecting an update loop -- i.e. to make sure
> the value actually got swapped (the "try" part...)?

The value has been updated, either by us or someone else.

We do not particularly care who won the race, since the value is
updated once only.

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Kees Cook]

On Thu, Nov 17, 2022 at 03:22:22PM -0800, Eric Dumazet wrote:
> On Thu, Nov 17, 2022 at 3:14 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Nov 17, 2022 at 02:49:55PM -0800, Eric Dumazet wrote:
> > > On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@chromium.org> wrote:
> > > >
> > > > Hello!
> > > >
> > > > This is an experimental semi-automated report about issues detected by
> > > > Coverity from a scan of next-20221117 as part of the linux-next scan project:
> > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > >
> > > > You're getting this email because you were associated with the identified
> > > > lines of code (noted below) that were touched by commits:
> > > >
> > > >   Wed Nov 16 12:42:01 2022 +0000
> > > >     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
> > > >
> > > > Coverity reported the following:
> > > >
> > > > *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > > net/core/sock_diag.c:33 in __sock_gen_cookie()
> > > > 27     {
> > > > 28      u64 res = atomic64_read(&sk->sk_cookie);
> > > > 29
> > > > 30      if (!res) {
> > > > 31              u64 new = gen_cookie_next(&sock_cookie);
> > > > 32
> > > > vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > > vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> > > > 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > >
> > >
> > > Hmmm. for some reason I thought @res was always updated...
> > >
> > > A fix would be to read sk->sk_cookie, but I guess your tool will still
> > > complain we do not care
> > > of  atomic64_try_cmpxchg() return value ?
> > >
> > > diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> > > index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
> > > 100644
> > > --- a/net/core/sock_diag.c
> > > +++ b/net/core/sock_diag.c
> > > @@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
> > >                 u64 new = gen_cookie_next(&sock_cookie);
> > >
> > >                 atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > > +               /* Another cpu/thread might have won the race,
> > > +                * reload the final value.
> > > +                */
> > > +               res = atomic64_read(&sk->sk_cookie);
> > >         }
> > >         return res;
> > >  }
> >
> > I think it's saying it was expecting an update loop -- i.e. to make sure
> > the value actually got swapped (the "try" part...)?
> 
> The value has been updated, either by us or someone else.
> 
> We do not particularly care who won the race, since the value is
> updated once only.

Ah! Okay, now I understand the added comment. Thanks :)

-- 
Kees Cook

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Eric Dumazet]

On Thu, Nov 17, 2022 at 4:25 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Nov 17, 2022 at 03:22:22PM -0800, Eric Dumazet wrote:
> > On Thu, Nov 17, 2022 at 3:14 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Thu, Nov 17, 2022 at 02:49:55PM -0800, Eric Dumazet wrote:
> > > > On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@chromium.org> wrote:
> > > > >
> > > > > Hello!
> > > > >
> > > > > This is an experimental semi-automated report about issues detected by
> > > > > Coverity from a scan of next-20221117 as part of the linux-next scan project:
> > > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > > >
> > > > > You're getting this email because you were associated with the identified
> > > > > lines of code (noted below) that were touched by commits:
> > > > >
> > > > >   Wed Nov 16 12:42:01 2022 +0000
> > > > >     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
> > > > >
> > > > > Coverity reported the following:
> > > > >
> > > > > *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > > > net/core/sock_diag.c:33 in __sock_gen_cookie()
> > > > > 27     {
> > > > > 28      u64 res = atomic64_read(&sk->sk_cookie);
> > > > > 29
> > > > > 30      if (!res) {
> > > > > 31              u64 new = gen_cookie_next(&sock_cookie);
> > > > > 32
> > > > > vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > > > vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> > > > > 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > > >
> > > >
> > > > Hmmm. for some reason I thought @res was always updated...
> > > >
> > > > A fix would be to read sk->sk_cookie, but I guess your tool will still
> > > > complain we do not care
> > > > of  atomic64_try_cmpxchg() return value ?
> > > >
> > > > diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> > > > index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
> > > > 100644
> > > > --- a/net/core/sock_diag.c
> > > > +++ b/net/core/sock_diag.c
> > > > @@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
> > > >                 u64 new = gen_cookie_next(&sock_cookie);
> > > >
> > > >                 atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > > > +               /* Another cpu/thread might have won the race,
> > > > +                * reload the final value.
> > > > +                */
> > > > +               res = atomic64_read(&sk->sk_cookie);
> > > >         }
> > > >         return res;
> > > >  }
> > >
> > > I think it's saying it was expecting an update loop -- i.e. to make sure
> > > the value actually got swapped (the "try" part...)?
> >
> > The value has been updated, either by us or someone else.
> >
> > We do not particularly care who won the race, since the value is
> > updated once only.
>
> Ah! Okay, now I understand the added comment. Thanks :)

I guess we could simply go back to atomic64_cmpxchg() to avoid a false positive.

This boils to avoid the loop we had prior to 4ebf802cf1c6

diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..7b9e321e0f6b15f2fb7af9f53fceb874439cbd02
100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -30,7 +30,11 @@ u64 __sock_gen_cookie(struct sock *sk)
        if (!res) {
                u64 new = gen_cookie_next(&sock_cookie);

-               atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
+               atomic64_cmpxchg(&sk->sk_cookie, res, new);
+               /* Another cpu/thread might have won the race,
+                * load the final value.
+                */
+               res = atomic64_read(&sk->sk_cookie);
        }
        return res;
 }

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Kees Cook]

On Thu, Nov 17, 2022 at 05:24:36PM -0800, Eric Dumazet wrote:
> On Thu, Nov 17, 2022 at 4:25 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Nov 17, 2022 at 03:22:22PM -0800, Eric Dumazet wrote:
> > > On Thu, Nov 17, 2022 at 3:14 PM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > On Thu, Nov 17, 2022 at 02:49:55PM -0800, Eric Dumazet wrote:
> > > > > On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@chromium.org> wrote:
> > > > > >
> > > > > > Hello!
> > > > > >
> > > > > > This is an experimental semi-automated report about issues detected by
> > > > > > Coverity from a scan of next-20221117 as part of the linux-next scan project:
> > > > > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > > > > >
> > > > > > You're getting this email because you were associated with the identified
> > > > > > lines of code (noted below) that were touched by commits:
> > > > > >
> > > > > >   Wed Nov 16 12:42:01 2022 +0000
> > > > > >     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
> > > > > >
> > > > > > Coverity reported the following:
> > > > > >
> > > > > > *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > > > > net/core/sock_diag.c:33 in __sock_gen_cookie()
> > > > > > 27     {
> > > > > > 28      u64 res = atomic64_read(&sk->sk_cookie);
> > > > > > 29
> > > > > > 30      if (!res) {
> > > > > > 31              u64 new = gen_cookie_next(&sock_cookie);
> > > > > > 32
> > > > > > vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > > > > vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> > > > > > 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > > > >
> > > > >
> > > > > Hmmm. for some reason I thought @res was always updated...
> > > > >
> > > > > A fix would be to read sk->sk_cookie, but I guess your tool will still
> > > > > complain we do not care
> > > > > of  atomic64_try_cmpxchg() return value ?
> > > > >
> > > > > diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> > > > > index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
> > > > > 100644
> > > > > --- a/net/core/sock_diag.c
> > > > > +++ b/net/core/sock_diag.c
> > > > > @@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
> > > > >                 u64 new = gen_cookie_next(&sock_cookie);
> > > > >
> > > > >                 atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > > > > +               /* Another cpu/thread might have won the race,
> > > > > +                * reload the final value.
> > > > > +                */
> > > > > +               res = atomic64_read(&sk->sk_cookie);
> > > > >         }
> > > > >         return res;
> > > > >  }
> > > >
> > > > I think it's saying it was expecting an update loop -- i.e. to make sure
> > > > the value actually got swapped (the "try" part...)?
> > >
> > > The value has been updated, either by us or someone else.
> > >
> > > We do not particularly care who won the race, since the value is
> > > updated once only.
> >
> > Ah! Okay, now I understand the added comment. Thanks :)
> 
> I guess we could simply go back to atomic64_cmpxchg() to avoid a false positive.

It looks like the existing code already works as intended, so no need to
silence the warning. The comment and reload might be nice to add, just
to clarify for anyone looking at it again in the future, though.

-- 
Kees Cook

Re: Coverity: __sock_gen_cookie(): Error handling issues

[Eric Dumazet]

On Thu, Nov 17, 2022 at 6:16 PM Kees Cook <keescook@chromium.org> wrote:

>
> It looks like the existing code already works as intended, so no need to
> silence the warning. The comment and reload might be nice to add, just
> to clarify for anyone looking at it again in the future, though.


The current code in net-next is broken, because if we succeed to
change sk->sk_cookie,
we return 0 (instead of @new). So your report was not a false positive.

Thanks.