Message-ID: <CA+55aFxoOh=G9DOphFXQD_4vR+skZX_N0g8XqM_BopY08GVHZQ@mail.gmail.com>
Date:	Sat, 30 Jul 2016 12:33:12 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Len Brown <lenb@...nel.org>
Cc:	sfi-devel@...plefirmware.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: sfi_core warnings - and a pointer arithmetic bug

Len,
 right now my tree finally contains only a couple of remaining
warnings, thanks to some objtool fixes and previous warning removal.

The two remaining warnings are both from SFI:

  drivers/sfi/sfi_core.c: In function ‘sfi_map_table’:
  drivers/sfi/sfi_core.c:175:53: warning: self-comparison always
evaluates to true [-Wtautological-compare]
    if (TABLE_ON_PAGE(th, th, th->len))
                                                       ^
  drivers/sfi/sfi_core.c: In function ‘sfi_unmap_table’:
  drivers/sfi/sfi_core.c:195:71: warning: self-comparison always
evaluates to true [-Wtautological-compare]
     sfi_unmap_memory(th, TABLE_ON_PAGE(th, th, th->len) ?
                                                                         ^

and my initial reaction was that those warnings are bogus, because
they simply come about from the SFI code using the same generic macro
to test whether a table is entirely contained on the same page as
another variable.

And yes, they really *are* bogus. The compiler is warning about
entirely immateral problems.

But the SFI code does have another macro that it could have used and
that would avoid the warning, so I decided that rather than disable
the warning, I'd just use that other helper macro that TABLE_ON_PAGE()
already expands to, and apply this:

-       if (TABLE_ON_PAGE(th, th, th->len))
+       if (ON_SAME_PAGE(th, th + th->len))
                return th;

-               sfi_unmap_memory(th, TABLE_ON_PAGE(th, th, th->len) ?
+               sfi_unmap_memory(th, ON_SAME_PAGE(th, th + th->len) ?
                                        sizeof(*th) : th->len);

and be done with it and have a clean build.

However, when I did that, the warnings do go away, but I refuse to
actually commit it, because the code _around_ that patch is entirely
and utterly buggy, and the warnings are the least of the problem with
it.

No, the real problem is that the is very very confused about what
"th->len" actually means!

Is "th->len" the size of the table in bytes? That's what it looks
like, and that's what all the mapping code basically assumes (ie
sfi_unmap_memory() takes a byte count, and we pass in th->len to it).

But that's not what the TABLE_ON_PAGE or ON_SAME_PAGE tests actually check!

They both just do pointer arithmetic: that whole "th + th->len" is not
adding "th->len" _bytes_ to the pointer, it's adding "th->len"
_entries_ to the pointer. That's true whether done internally in the
TABLE_ON_PAGE() expansion, or done explicitly when rewriting it to use
"ON_SAME_PAGE(th, th + th->len)" to avoid the self-comparison.

So I removed my patch to fix the warning, because there is a much more
important fix that needs to be done to that code.

Maybe "ON_SAME_PAGE()" should take an offset as a third argument, and
add that offset to the pointer _after_ having cast the pointer into an
"unsigned long". Then the "is it on the same page" test would become

   ON_SAME_PAGE(th, th, th->len)

and you'd make

  #define TABLE_ON_PAGE(page, table, size) \
     (ON_SAME_PAGE(page, table, 0) && \
      ON_SAME_PAGE(page, table, size))

or something.

But this needs actual testing, and somebody who cares about SFI. It's
a bigger change than just fixing a warning, since it actually fixes
the code to do something different.

                  Linus

Powered by blists - more mailing lists