Beginning with 553f770ef71b, the following program fails when compiled
for 32 bit and executed on a 64 bit kernel and succeeds when compiled
for and executed on a 64 bit. It continues to fail even after
58aff0af7573. When compiled as 32 bit, an shmctl call fails with
EBADR (see the XXX comment).
The test program is adapted from rr's shm test[0].
#define _GNU_SOURCE 1
#include <assert.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/shm.h>
#include <sys/wait.h>
static uint64_t GUARD_VALUE = 0xdeadbeeff00dbaad;
inline static size_t ceil_page_size(size_t size) {
size_t page_size = sysconf(_SC_PAGESIZE);
return (size + page_size - 1) & ~(page_size - 1);
}
/**
* Allocate 'size' bytes, fill with 'value', place canary value before
* the allocated block, and put guard pages before and after. Ensure
* there's a guard page immediately after `size`.
* This lets us catch cases where too much data is being recorded --- which can
* cause errors if the recorder tries to read invalid memory.
*/
inline static void* allocate_guard(size_t size, char value) {
size_t page_size = sysconf(_SC_PAGESIZE);
size_t map_size = ceil_page_size(size + sizeof(GUARD_VALUE)) + 2 * page_size;
char* cp = (char*)mmap(NULL, map_size, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
assert(cp != MAP_FAILED);
/* create guard pages */
assert(munmap(cp, page_size) == 0);
assert(munmap(cp + map_size - page_size, page_size) == 0);
cp = cp + map_size - page_size - size;
memcpy(cp - sizeof(GUARD_VALUE), &GUARD_VALUE, sizeof(GUARD_VALUE));
memset(cp, value, size);
return cp;
}
/**
* Verify that canary value before the block allocated at 'p'
* (of size 'size') is still valid.
*/
inline static void verify_guard(__attribute__((unused)) size_t size, void* p) {
char* cp = (char*)p;
assert(memcmp(cp - sizeof(GUARD_VALUE), &GUARD_VALUE,
sizeof(GUARD_VALUE)) == 0);
}
/**
* Verify that canary value before the block allocated at 'p'
* (of size 'size') is still valid, and free the block.
*/
inline static void free_guard(size_t size, void* p) {
verify_guard(size, p);
size_t page_size = sysconf(_SC_PAGESIZE);
size_t map_size = ceil_page_size(size + sizeof(GUARD_VALUE)) + 2 * page_size;
char* cp = (char*)p + size + page_size - map_size;
assert(0 == munmap(cp, map_size - 2 * page_size));
}
#define ALLOCATE_GUARD(p, v) p = allocate_guard(sizeof(*p), v)
#define VERIFY_GUARD(p) verify_guard(sizeof(*p), p)
#define FREE_GUARD(p) free_guard(sizeof(*p), p)
/* Make SIZE not a multiple of the page size, to ensure we handle that case.
But make sure it's even, since we divide it by two. */
#define SIZE ((int)(16 * page_size) - 10)
static int shmid;
static void before_writing(void) {}
static void after_writing(void) {}
static int run_child(void) {
int i;
char* p;
char* p2;
pid_t child2;
int status;
struct shmid_ds* ds;
struct shminfo* info;
struct shm_info* info2;
size_t page_size = sysconf(_SC_PAGESIZE);
ALLOCATE_GUARD(ds, 'd');
assert(0 == shmctl(shmid, IPC_STAT, ds));
VERIFY_GUARD(ds);
assert((int)ds->shm_segsz == SIZE);
assert(ds->shm_cpid == getppid());
assert(ds->shm_nattch == 0);
ds->shm_perm.mode = 0660;
assert(0 == shmctl(shmid, IPC_SET, ds));
ALLOCATE_GUARD(info, 'i');
assert(0 <= shmctl(shmid, IPC_INFO, (struct shmid_ds*)info));
VERIFY_GUARD(info);
assert(info->shmmin == 1);
ALLOCATE_GUARD(info2, 'j');
// XXX: This shmctl call fails with EBADR when compiled with -m32
assert(0 <= shmctl(shmid, SHM_INFO, (struct shmid_ds*)info2));
VERIFY_GUARD(info2);
assert(info2->used_ids > 0);
assert(info2->used_ids < 1000000);
p = shmat(shmid, NULL, 0);
assert(p != (char*)-1);
before_writing();
for (i = 0; i < SIZE; ++i) {
assert(p[i] == 0);
}
memset(p, 'r', SIZE / 2);
after_writing();
p2 = shmat(shmid, NULL, 0);
assert(p2 != (char*)-1);
memset(p + SIZE / 2, 'r', SIZE / 2);
assert(0 == shmdt(p));
assert(0 == shmdt(p2));
assert(p == mmap(p, SIZE, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0));
assert(p[0] == 0);
p = shmat(shmid, p, SHM_REMAP);
assert(p != (char*)-1);
for (i = 0; i < SIZE; ++i) {
assert(p[i] == 'r');
}
if ((child2 = fork()) == 0) {
memset(p, 's', SIZE);
return 0;
}
assert(child2 == waitpid(child2, &status, __WALL));
assert(0 == status);
for (i = 0; i < SIZE; ++i) {
assert(p[i] == 's');
}
return 0;
}
int main(void) {
pid_t child;
int status;
size_t page_size = sysconf(_SC_PAGESIZE);
shmid = shmget(IPC_PRIVATE, SIZE, 0666);
assert(shmid >= 0);
if ((child = fork()) == 0) {
return run_child();
}
printf("child %d\n", child);
assert(child == waitpid(child, &status, __WALL));
/* delete the shm before testing status, because we want to ensure the
segment is deleted even if the test failed. */
assert(0 == shmctl(shmid, IPC_RMID, NULL));
assert(status == 0);
printf("EXIT-SUCCESS\n");
return 0;
}
- Kyle
[0] https://github.com/mozilla/rr/blob/master/src/test/shm.c
On Mon, Sep 25, 2017 at 03:18:29PM -0700, Kyle Huey wrote:
> Beginning with 553f770ef71b, the following program fails when compiled
> for 32 bit and executed on a 64 bit kernel and succeeds when compiled
> for and executed on a 64 bit. It continues to fail even after
> 58aff0af7573. When compiled as 32 bit, an shmctl call fails with
> EBADR (see the XXX comment).
Egads...
static int put_compat_shm_info(struct shm_info *ip,
struct compat_shm_info __user *uip)
{
struct compat_shm_info info;
memset(&info, 0, sizeof(info));
info.used_ids = ip->used_ids;
info.shm_tot = ip->shm_tot;
info.shm_rss = ip->shm_rss;
info.shm_swp = ip->shm_swp;
info.swap_attempts = ip->swap_attempts;
info.swap_successes = ip->swap_successes;
return copy_to_user(up, &info, sizeof(info));
^^
This.
I really wish gcc warned about conversions from pointer to function into
void *...
Linus, could you pull that?
The following changes since commit 58aff0af757356065f33290d96a9cd46dfbcae88:
ipc/shm: Fix order of parameters when calling copy_compat_shmid_to_user (2017-09-20 23:27:48 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus
for you to fetch changes up to b776e4b1a990045a7c70798f1f353c3160c26594:
fix a typo in put_compat_shm_info() (2017-09-25 20:41:46 -0400)
----------------------------------------------------------------
Al Viro (1):
fix a typo in put_compat_shm_info()
ipc/shm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On Mon, Sep 25, 2017 at 6:00 PM, Al Viro <[email protected]> wrote:
>
> I really wish gcc warned about conversions from pointer to function into
> void *...
Pulled and pushed out, but I'd like to note that sparse would have
caught this. Except we are so far away from being sparse-clean that
nobody runs it.
And I think your recent compat cleanup work actually made it worse,
showing new warnings (including the one that was a real bug)
Oh well.
Patch to at least fix the address space warnings in ipc/ attached.
Linus
On Mon, Sep 25, 2017 at 06:37:28PM -0700, Linus Torvalds wrote:
> And I think your recent compat cleanup work actually made it worse,
> showing new warnings (including the one that was a real bug)
Actually, they are not new - try make C=2 ipc/compat.o on v4.13 and you'll
see their previous locations.
> Patch to at least fix the address space warnings in ipc/ attached.
Which tree do you prefer it to go through? Direct to mainline, or vfs.git
#for-next?
On Tue, Sep 26, 2017 at 02:46:56AM +0100, Al Viro wrote:
> On Mon, Sep 25, 2017 at 06:37:28PM -0700, Linus Torvalds wrote:
>
> > And I think your recent compat cleanup work actually made it worse,
> > showing new warnings (including the one that was a real bug)
>
> Actually, they are not new - try make C=2 ipc/compat.o on v4.13 and you'll
> see their previous locations.
>
> > Patch to at least fix the address space warnings in ipc/ attached.
>
> Which tree do you prefer it to go through? Direct to mainline, or vfs.git
> #for-next?
FWIW, __percpu and __rcu annotations are messy as hell. Never got around
to sorting down the infrastructure annotations for that bunch, and I'm
not entirely sure that they (especially __rcu) are a good match for
__address_space__(()).
On Mon, Sep 25, 2017 at 6:46 PM, Al Viro <[email protected]> wrote:
>
> Which tree do you prefer it to go through? Direct to mainline, or vfs.git
> #for-next?
for-next, it's not like it's in any way urgent.
Linus
On Mon, Sep 25, 2017 at 7:00 PM, Al Viro <[email protected]> wrote:
>
> FWIW, __percpu and __rcu annotations are messy as hell. Never got around
> to sorting down the infrastructure annotations for that bunch, and I'm
> not entirely sure that they (especially __rcu) are a good match for
> __address_space__(()).
I agree. It might be better to just remove the address space logic,
because afaik it never worked for them.
Linus
On Mon, Sep 25, 2017 at 7:03 PM, Linus Torvalds
<[email protected]> wrote:
>
> I agree. It might be better to just remove the address space logic,
> because afaik it never worked for them.
.. and sadly, we should probably disable the locking ones by default
too, because while they *work*, sparse only handles static cases, and
we have way too many dynamically conditional cases that are outside
the scope of what sparse does.
It would probably be good to disable things that are fundamentally
hard to fix, and aim for a clean sparse build, and maybe people would
start using it at least for user pointer checking where it really does
work.
Of course, even there it depends on pointers _statically_ being user
pointers, but happily we do largely follow that rule. We've had a few
nasty cases where we have a pointer that is conditionally user or
kernel pointer, but they are thankfully pretty rare.
Linus
On Mon, Sep 25, 2017 at 07:07:01PM -0700, Linus Torvalds wrote:
> On Mon, Sep 25, 2017 at 7:03 PM, Linus Torvalds
> <[email protected]> wrote:
> >
> > I agree. It might be better to just remove the address space logic,
> > because afaik it never worked for them.
>
> .. and sadly, we should probably disable the locking ones by default
> too, because while they *work*, sparse only handles static cases, and
> we have way too many dynamically conditional cases that are outside
> the scope of what sparse does.
>
> It would probably be good to disable things that are fundamentally
> hard to fix, and aim for a clean sparse build, and maybe people would
> start using it at least for user pointer checking where it really does
> work.
>
> Of course, even there it depends on pointers _statically_ being user
> pointers, but happily we do largely follow that rule. We've had a few
> nasty cases where we have a pointer that is conditionally user or
> kernel pointer, but they are thankfully pretty rare.
BTW, while we are at it - I'd been rebasing POLL... annotations through
the last three cycles and it doesn't take much work (usually 20-30
minutes). Mind if I throw vfs.git#misc.poll into -next and send it
your way next cycle?
Right now it's pretty much in zero-noise state - a few of the remaining
warnings are spurious, but most of what remains consists of real bugs.
One class is ->poll() instance returning -E... in some case; callers
expect a bitmap instead. Another, and that's much nastier, is EPOLL...
mess. We have EPOLL... definitions identical for all architectures.
Unfortunately, we rely upon them being equal to corresponding POLL...
(when both are defined) and some of those are different on different
architectures (sparc is the strangest one in that respect). Both are
exposed to userland, so we can't just go and change them at will.
Not sure what can be done with that, syscall ABI being what it is...
On Mon, Sep 25, 2017 at 06:37:28PM -0700, Linus Torvalds wrote:
> Pulled and pushed out, but I'd like to note that sparse would have
> caught this. Except we are so far away from being sparse-clean that
> nobody runs it.
I tend to run sparse over the nvme code before sending pull request
every time. But it's a fairly new codebase, so it it actually
is clean. I wish we'd just default to running sparse at some point
so people have to clean their shit up, as it catches a lot of
useful things. But maybe for the default we want to tune it down
a bit (e.g. don't warn about missing statics by default, skip
the lock imbalance checks which while often useful also generate
tons of false positives).
On Mon, Sep 25, 2017 at 07:07:01PM -0700, Linus Torvalds wrote:
> On Mon, Sep 25, 2017 at 7:03 PM, Linus Torvalds
> <[email protected]> wrote:
> >
> > I agree. It might be better to just remove the address space logic,
> > because afaik it never worked for them.
>
> .. and sadly, we should probably disable the locking ones by default
> too, because while they *work*, sparse only handles static cases, and
> we have way too many dynamically conditional cases that are outside
> the scope of what sparse does.
>
> It would probably be good to disable things that are fundamentally
> hard to fix, and aim for a clean sparse build, and maybe people would
> start using it at least for user pointer checking where it really does
> work.
Currently, the most important cause of uncleanness regarding sparse build,
and by far, is restricted types/-Wbitwise, presumably mostly __be/__le issues.
For example, on 4.13/x86-64/allyesconfig, the summary of sparse errors &
warnings is something like (with details removed):
11096 cast to restricted type
6036 incorrect type in assignment (different base types)
3439 incorrect type in initializer (different base types)
2774 symbol was not declared. Should it be static?
1670 incorrect type in argument (different address spaces)
1284 restricted type degrades to integer
889 cast from restricted type
790 incorrect type in argument (different base types)
668 cast removes address space of expression
600 incompatible types in comparison expression (different address spaces)
558 invalid assignement
465 incorrect type in assignment (different address spaces)
397 context imbalance in - unexpected unlock
297 Variable length array is used.
261 dereference of noderef expression
250 incorrect type in initializer (different address spaces)
244 context imbalance in - different lock contexts for basic block
228 attribute 'require_context': unknown attribute
201 invalid bitfield specifier for type restricted type.
164 context imbalance in - wrong count at exit
104 directive in argument list
93 Using plain integer as NULL pointer
85 incorrect type in return expression (different address spaces)
70 bad integer constant expression
69 cast truncates bits from constant value
55 Initializer entry defined twice
49 mixing different enum types
48 incorrect type in return expression (different base types)
23 constant is so big it is ...
23 cannot size expression
20 function with external linkage has definition
19 preprocessor token offsetof redefined
18 advancing past deep designator
17 no newline at end of file
14 incorrect type in argument (different modifiers)
13 bad assignment to restricted type
12 symbol redeclared with different type - different modifiers
11 dubious: x | !y
10 preprocessor token __must_hold redefined
10 "Sparse checking disabled for this file"
8 call with no type!
7 subtraction of functions? Share your drugs
6 subtraction of different types can't work (different address spaces)
6 shift too big for type
6 invalid initializer
5 incorrect type in initializer (different modifiers)
5 cast to non-scalar
4 trying to concatenate long character string (8191 bytes max)
4 right shift by bigger than source value
4 dubious: !x | y
3 missing braces around initializer
3 incorrect type in initializer (incompatible argument (different address spaces))
3 incompatible types in conditional expression (different base types)
3 dubious: x & !y
2 symbol redeclared with different type - incompatible argument (different address spaces)
2 switch with no cases
2 incorrect type in assignment (different modifiers)
2 incompatible types in comparison expression (different base types)
2 implicit cast from nocast type
2 dubious: !x & y
2 division by zero
2 cast between address spaces (<asn:3>-><asn:4>)
2 arithmetics on pointers to functions
1 unknown expression (8 46)
1 too long token expansion
1 incorrect type in initializer (incompatible argument (different signedness))
1 incorrect type in conditional
1 incorrect type in argument (incompatible argument (different signedness))
1 incorrect type in argument (incompatible argument (different base types))
1 incompatible types in conditional expression (different types)
1 cast from non-scalar
1 bad argument type for ++/--
-- Luc Van Oostenryck
Christoph Hellwig <[email protected]> writes:
> On Mon, Sep 25, 2017 at 06:37:28PM -0700, Linus Torvalds wrote:
>> Pulled and pushed out, but I'd like to note that sparse would have
>> caught this. Except we are so far away from being sparse-clean that
>> nobody runs it.
>
> I tend to run sparse over the nvme code before sending pull request
> every time. But it's a fairly new codebase, so it it actually
> is clean. I wish we'd just default to running sparse at some point
> so people have to clean their shit up, as it catches a lot of
> useful things. But maybe for the default we want to tune it down
> a bit (e.g. don't warn about missing statics by default, skip
> the lock imbalance checks which while often useful also generate
> tons of false positives).
Daniel (++Cc) wrote a script a while back that can do a "smart" diff of
the sparse output from two builds. Roughly it sorts the output
(important when using make -j) and does some other munging to try and
give you a minimal diff across runs.
That allows you to check if a commit added new sparse warnings without
the build being clean at the beginning.
Anyway it's here if anyone wants to try it:
https://github.com/daxtens/smart-sparse-diff
cheers