All of lore.kernel.org
 help / color / mirror / Atom feed
* Regression related to ipc shmctl compat
@ 2017-09-25 22:18 Kyle Huey
  2017-09-26  1:00 ` [git pull] vfs.git regression fix " Al Viro
  0 siblings, 1 reply; 17+ messages in thread
From: Kyle Huey @ 2017-09-25 22:18 UTC (permalink / raw)
  To: open list, Al Viro; +Cc: Robert O'Callahan

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-25 22:18 Regression related to ipc shmctl compat Kyle Huey
@ 2017-09-26  1:00 ` Al Viro
  2017-09-26  1:37   ` Linus Torvalds
  2017-10-15  6:58   ` [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat Pavel Machek
  0 siblings, 2 replies; 17+ messages in thread
From: Al Viro @ 2017-09-26  1:00 UTC (permalink / raw)
  To: Kyle Huey; +Cc: open list, Robert O'Callahan, Linus Torvalds

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(-)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  1:00 ` [git pull] vfs.git regression fix " Al Viro
@ 2017-09-26  1:37   ` Linus Torvalds
  2017-09-26  1:46     ` Al Viro
  2017-09-26  6:42     ` Christoph Hellwig
  2017-10-15  6:58   ` [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat Pavel Machek
  1 sibling, 2 replies; 17+ messages in thread
From: Linus Torvalds @ 2017-09-26  1:37 UTC (permalink / raw)
  To: Al Viro; +Cc: Kyle Huey, open list, Robert O'Callahan

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On Mon, Sep 25, 2017 at 6:00 PM, Al Viro <viro@zeniv.linux.org.uk> 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

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2481 bytes --]

 ipc/msg.c     | 4 ++--
 ipc/sem.c     | 4 ++--
 ipc/shm.c     | 4 ++--
 ipc/syscall.c | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 06be5a9adfa4..ebb7ea24ee28 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -590,13 +590,13 @@ static int copy_compat_msqid_from_user(struct msqid64_ds *out, void __user *buf,
 {
 	memset(out, 0, sizeof(*out));
 	if (version == IPC_64) {
-		struct compat_msqid64_ds *p = buf;
+		struct compat_msqid64_ds __user *p = buf;
 		if (get_compat_ipc64_perm(&out->msg_perm, &p->msg_perm))
 			return -EFAULT;
 		if (get_user(out->msg_qbytes, &p->msg_qbytes))
 			return -EFAULT;
 	} else {
-		struct compat_msqid_ds *p = buf;
+		struct compat_msqid_ds __user *p = buf;
 		if (get_compat_ipc_perm(&out->msg_perm, &p->msg_perm))
 			return -EFAULT;
 		if (get_user(out->msg_qbytes, &p->msg_qbytes))
diff --git a/ipc/sem.c b/ipc/sem.c
index f7385bce5fd3..6220e9616207 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1636,10 +1636,10 @@ static int copy_compat_semid_from_user(struct semid64_ds *out, void __user *buf,
 {
 	memset(out, 0, sizeof(*out));
 	if (version == IPC_64) {
-		struct compat_semid64_ds *p = buf;
+		struct compat_semid64_ds __user *p = buf;
 		return get_compat_ipc64_perm(&out->sem_perm, &p->sem_perm);
 	} else {
-		struct compat_semid_ds *p = buf;
+		struct compat_semid_ds __user *p = buf;
 		return get_compat_ipc_perm(&out->sem_perm, &p->sem_perm);
 	}
 }
diff --git a/ipc/shm.c b/ipc/shm.c
index badac463e2c8..85428b094df9 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1193,10 +1193,10 @@ static int copy_compat_shmid_from_user(struct shmid64_ds *out, void __user *buf,
 {
 	memset(out, 0, sizeof(*out));
 	if (version == IPC_64) {
-		struct compat_shmid64_ds *p = buf;
+		struct compat_shmid64_ds __user *p = buf;
 		return get_compat_ipc64_perm(&out->shm_perm, &p->shm_perm);
 	} else {
-		struct compat_shmid_ds *p = buf;
+		struct compat_shmid_ds __user *p = buf;
 		return get_compat_ipc_perm(&out->shm_perm, &p->shm_perm);
 	}
 }
diff --git a/ipc/syscall.c b/ipc/syscall.c
index 667022746ca5..977bffd5a7f8 100644
--- a/ipc/syscall.c
+++ b/ipc/syscall.c
@@ -171,7 +171,7 @@ COMPAT_SYSCALL_DEFINE6(ipc, u32, call, int, first, int, second,
 			       COMPAT_SHMLBA);
 		if (err < 0)
 			return err;
-		return put_user(raddr, (compat_ulong_t *)compat_ptr(third));
+		return put_user(raddr, (compat_ulong_t __user *)compat_ptr(third));
 	}
 	case SHMDT:
 		return sys_shmdt(compat_ptr(ptr));

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  1:37   ` Linus Torvalds
@ 2017-09-26  1:46     ` Al Viro
  2017-09-26  2:00       ` Al Viro
  2017-09-26  2:02       ` Linus Torvalds
  2017-09-26  6:42     ` Christoph Hellwig
  1 sibling, 2 replies; 17+ messages in thread
From: Al Viro @ 2017-09-26  1:46 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Kyle Huey, open list, Robert O'Callahan

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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  1:46     ` Al Viro
@ 2017-09-26  2:00       ` Al Viro
  2017-09-26  2:03         ` Linus Torvalds
  2017-09-26  2:02       ` Linus Torvalds
  1 sibling, 1 reply; 17+ messages in thread
From: Al Viro @ 2017-09-26  2:00 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Kyle Huey, open list, Robert O'Callahan

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__(()).

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  1:46     ` Al Viro
  2017-09-26  2:00       ` Al Viro
@ 2017-09-26  2:02       ` Linus Torvalds
  2017-10-11 17:03         ` Al Viro
  1 sibling, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2017-09-26  2:02 UTC (permalink / raw)
  To: Al Viro; +Cc: Kyle Huey, open list, Robert O'Callahan

On Mon, Sep 25, 2017 at 6:46 PM, Al Viro <viro@zeniv.linux.org.uk> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  2:00       ` Al Viro
@ 2017-09-26  2:03         ` Linus Torvalds
  2017-09-26  2:07           ` Linus Torvalds
  0 siblings, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2017-09-26  2:03 UTC (permalink / raw)
  To: Al Viro; +Cc: Kyle Huey, open list, Robert O'Callahan

On Mon, Sep 25, 2017 at 7:00 PM, Al Viro <viro@zeniv.linux.org.uk> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  2:03         ` Linus Torvalds
@ 2017-09-26  2:07           ` Linus Torvalds
  2017-09-26  3:01             ` Al Viro
  2017-09-26 19:45             ` Luc Van Oostenryck
  0 siblings, 2 replies; 17+ messages in thread
From: Linus Torvalds @ 2017-09-26  2:07 UTC (permalink / raw)
  To: Al Viro; +Cc: Kyle Huey, open list, Robert O'Callahan

On Mon, Sep 25, 2017 at 7:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  2:07           ` Linus Torvalds
@ 2017-09-26  3:01             ` Al Viro
  2017-09-26 19:45             ` Luc Van Oostenryck
  1 sibling, 0 replies; 17+ messages in thread
From: Al Viro @ 2017-09-26  3:01 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Kyle Huey, open list, Robert O'Callahan

On Mon, Sep 25, 2017 at 07:07:01PM -0700, Linus Torvalds wrote:
> On Mon, Sep 25, 2017 at 7:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  1:37   ` Linus Torvalds
  2017-09-26  1:46     ` Al Viro
@ 2017-09-26  6:42     ` Christoph Hellwig
  2017-09-28  6:13       ` Script to do smart sparse diffs (was Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat) Michael Ellerman
  1 sibling, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2017-09-26  6:42 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Al Viro, Kyle Huey, open list, Robert O'Callahan

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).

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  2:07           ` Linus Torvalds
  2017-09-26  3:01             ` Al Viro
@ 2017-09-26 19:45             ` Luc Van Oostenryck
  1 sibling, 0 replies; 17+ messages in thread
From: Luc Van Oostenryck @ 2017-09-26 19:45 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Al Viro, Kyle Huey, open list, Robert O'Callahan

On Mon, Sep 25, 2017 at 07:07:01PM -0700, Linus Torvalds wrote:
> On Mon, Sep 25, 2017 at 7:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Script to do smart sparse diffs (was Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat)
  2017-09-26  6:42     ` Christoph Hellwig
@ 2017-09-28  6:13       ` Michael Ellerman
  0 siblings, 0 replies; 17+ messages in thread
From: Michael Ellerman @ 2017-09-28  6:13 UTC (permalink / raw)
  To: Christoph Hellwig, Linus Torvalds, Daniel Axtens
  Cc: Al Viro, Kyle Huey, open list, Robert O'Callahan

Christoph Hellwig <hch@infradead.org> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  2:02       ` Linus Torvalds
@ 2017-10-11 17:03         ` Al Viro
  2017-10-11 17:06           ` Al Viro
  2017-10-11 17:31           ` Linus Torvalds
  0 siblings, 2 replies; 17+ messages in thread
From: Al Viro @ 2017-10-11 17:03 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Kyle Huey, open list, Robert O'Callahan

On Mon, Sep 25, 2017 at 07:02:16PM -0700, Linus Torvalds wrote:
> On Mon, Sep 25, 2017 at 6:46 PM, Al Viro <viro@zeniv.linux.org.uk> 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.

Can I assume your normal S-o-b on that?  Just noticed that thing sitting
in misc queue with mismatched Author: and Signed-off-by:...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-10-11 17:03         ` Al Viro
@ 2017-10-11 17:06           ` Al Viro
  2017-10-11 17:31           ` Linus Torvalds
  1 sibling, 0 replies; 17+ messages in thread
From: Al Viro @ 2017-10-11 17:06 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Kyle Huey, open list, Robert O'Callahan

On Wed, Oct 11, 2017 at 06:03:35PM +0100, Al Viro wrote:
> On Mon, Sep 25, 2017 at 07:02:16PM -0700, Linus Torvalds wrote:
> > On Mon, Sep 25, 2017 at 6:46 PM, Al Viro <viro@zeniv.linux.org.uk> 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.
> 
> Can I assume your normal S-o-b on that?  Just noticed that thing sitting
> in misc queue with mismatched Author: and Signed-off-by:...

FWIW, what I've got in there is

Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Sep 25 18:37:28 2017 -0700

    fix address space warnings in ipc/
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/ipc/msg.c b/ipc/msg.c
index 06be5a9adfa4..ebb7ea24ee28 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -590,13 +590,13 @@ static int copy_compat_msqid_from_user(struct msqid64_ds *out, void __user *buf,
 {
 	memset(out, 0, sizeof(*out));
 	if (version == IPC_64) {
-		struct compat_msqid64_ds *p = buf;
+		struct compat_msqid64_ds __user *p = buf;
 		if (get_compat_ipc64_perm(&out->msg_perm, &p->msg_perm))
 			return -EFAULT;
 		if (get_user(out->msg_qbytes, &p->msg_qbytes))
 			return -EFAULT;
 	} else {
-		struct compat_msqid_ds *p = buf;
+		struct compat_msqid_ds __user *p = buf;
 		if (get_compat_ipc_perm(&out->msg_perm, &p->msg_perm))
 			return -EFAULT;
 		if (get_user(out->msg_qbytes, &p->msg_qbytes))
diff --git a/ipc/sem.c b/ipc/sem.c
index f7385bce5fd3..6220e9616207 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1636,10 +1636,10 @@ static int copy_compat_semid_from_user(struct semid64_ds *out, void __user *buf,
 {
 	memset(out, 0, sizeof(*out));
 	if (version == IPC_64) {
-		struct compat_semid64_ds *p = buf;
+		struct compat_semid64_ds __user *p = buf;
 		return get_compat_ipc64_perm(&out->sem_perm, &p->sem_perm);
 	} else {
-		struct compat_semid_ds *p = buf;
+		struct compat_semid_ds __user *p = buf;
 		return get_compat_ipc_perm(&out->sem_perm, &p->sem_perm);
 	}
 }
diff --git a/ipc/shm.c b/ipc/shm.c
index 1b3adfe3c60e..41706416a3c4 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1193,10 +1193,10 @@ static int copy_compat_shmid_from_user(struct shmid64_ds *out, void __user *buf,
 {
 	memset(out, 0, sizeof(*out));
 	if (version == IPC_64) {
-		struct compat_shmid64_ds *p = buf;
+		struct compat_shmid64_ds __user *p = buf;
 		return get_compat_ipc64_perm(&out->shm_perm, &p->shm_perm);
 	} else {
-		struct compat_shmid_ds *p = buf;
+		struct compat_shmid_ds __user *p = buf;
 		return get_compat_ipc_perm(&out->shm_perm, &p->shm_perm);
 	}
 }
diff --git a/ipc/syscall.c b/ipc/syscall.c
index 667022746ca5..977bffd5a7f8 100644
--- a/ipc/syscall.c
+++ b/ipc/syscall.c
@@ -171,7 +171,7 @@ COMPAT_SYSCALL_DEFINE6(ipc, u32, call, int, first, int, second,
 			       COMPAT_SHMLBA);
 		if (err < 0)
 			return err;
-		return put_user(raddr, (compat_ulong_t *)compat_ptr(third));
+		return put_user(raddr, (compat_ulong_t __user *)compat_ptr(third));
 	}
 	case SHMDT:
 		return sys_shmdt(compat_ptr(ptr));

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-10-11 17:03         ` Al Viro
  2017-10-11 17:06           ` Al Viro
@ 2017-10-11 17:31           ` Linus Torvalds
  1 sibling, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2017-10-11 17:31 UTC (permalink / raw)
  To: Al Viro; +Cc: Kyle Huey, open list, Robert O'Callahan

On Wed, Oct 11, 2017 at 10:03 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Can I assume your normal S-o-b on that?  Just noticed that thing sitting
> in misc queue with mismatched Author: and Signed-off-by:...

Yup, just add my sign-off.

Thanks,
                Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-09-26  1:00 ` [git pull] vfs.git regression fix " Al Viro
  2017-09-26  1:37   ` Linus Torvalds
@ 2017-10-15  6:58   ` Pavel Machek
  2017-10-16 11:41     ` Linus Torvalds
  1 sibling, 1 reply; 17+ messages in thread
From: Pavel Machek @ 2017-10-15  6:58 UTC (permalink / raw)
  To: Al Viro; +Cc: Kyle Huey, open list, Robert O'Callahan, Linus Torvalds

On Tue 2017-09-26 02:00:36, Al Viro wrote:
> 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 *...

up is quite short and not-specific for global symbol. Rename to mutex_up?

									Pavel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat
  2017-10-15  6:58   ` [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat Pavel Machek
@ 2017-10-16 11:41     ` Linus Torvalds
  0 siblings, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2017-10-16 11:41 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Al Viro, Kyle Huey, open list, Robert O'Callahan

On Sun, Oct 15, 2017 at 2:58 AM, Pavel Machek <pavel@ucw.cz> wrote:
>
> up is quite short and not-specific for global symbol. Rename to mutex_up?

Well, it is quite the traditional name in the OS community and in
Linux it goes back to before the 1.0 release.

We've got about 450 each of the up() and the down() (with varieties:
down_interruptible/killable/trylock), so I guess it wouldn't be *too*
painful to change them.

The prefix wouldn't be "mutex_", though, these are the traditional old
counting semaphores.

              Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-10-16 11:41 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-25 22:18 Regression related to ipc shmctl compat Kyle Huey
2017-09-26  1:00 ` [git pull] vfs.git regression fix " Al Viro
2017-09-26  1:37   ` Linus Torvalds
2017-09-26  1:46     ` Al Viro
2017-09-26  2:00       ` Al Viro
2017-09-26  2:03         ` Linus Torvalds
2017-09-26  2:07           ` Linus Torvalds
2017-09-26  3:01             ` Al Viro
2017-09-26 19:45             ` Luc Van Oostenryck
2017-09-26  2:02       ` Linus Torvalds
2017-10-11 17:03         ` Al Viro
2017-10-11 17:06           ` Al Viro
2017-10-11 17:31           ` Linus Torvalds
2017-09-26  6:42     ` Christoph Hellwig
2017-09-28  6:13       ` Script to do smart sparse diffs (was Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat) Michael Ellerman
2017-10-15  6:58   ` [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat Pavel Machek
2017-10-16 11:41     ` Linus Torvalds

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.