On Sun, Jul 01, 2018 at 08:51:55PM +0200, Julia Lawall wrote:

On Sun, 1 Jul 2018, Joe Perches wrote:

quoted

On Sun, 2018-07-01 at 19:32 +0200, Julia Lawall wrote:

quoted

Comparing an int to a size, which is unsigned, causes the int to become
unsigned, giving the wrong result.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

Great, thanks.

But what about the ones in net/smc like:

quoted

net/smc/smc_clc.c:

        len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
                             sizeof(struct smc_clc_msg_decline));
        if (len < sizeof(struct smc_clc_msg_decline))

Are those detected by the semantic match and ignored?

I wasn't sure how to justify that kernel_sendmsg returns a negative value.
If it is the case, I can send the patch.  I only found this in one file,
but there were multiple occurrences.

In theory, Smatch is supposed to know return values but kernel_sendmsg()
is too complicated for Smatch.  It's a tricky thing...  That particular
check is correct and deliberate, but there is another check which is
wrong.

net/smc/smc_clc.c
   369          len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
   370                               sizeof(struct smc_clc_msg_decline));
   371          if (len < sizeof(struct smc_clc_msg_decline))
   372                  smc->sk.sk_err = EPROTO;
   373          if (len < 0)
   374                  smc->sk.sk_err = -len;

If it's invalid we set an error code, if it's already an error we
preserve the error code.

   375          return sock_error(&smc->sk);

[ snip ]

   442          /* due to the few bytes needed for clc-handshake this cannot block */
   443          len = kernel_sendmsg(smc->clcsock, &msg, vec, i, plen);
   444          if (len < sizeof(pclc)) {
   445                  if (len >= 0) {
                            ^^^^^^^^
This is always true.

   446                          reason_code = -ENETUNREACH;
   447                          smc->sk.sk_err = -reason_code;
   448                  } else {
   449                          smc->sk.sk_err = smc->clcsock->sk->sk_err;
   450                          reason_code = -smc->sk.sk_err;
   451                  }
   452          }

The other two checks are not type promoted so they also work as
intended.

This is an interesting sort of bug I've written a Smatch script inspired
by your work here.  One for the type promotion and one for the
impossible condition.  I'll let you know how it goes.

regards,
dan carpenter