Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1739314 > unrolled thread

Regression related to ipc shmctl compat

Back to article view | Back to linux.kernel

Contents

  Regression related to ipc shmctl compat Kyle Huey <me@kylehuey.com> - 2017-09-26 00:20 +0200
    [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Al Viro <viro@ZenIV.linux.org.uk> - 2017-09-26 03:10 +0200
      Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-26 03:40 +0200
        Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Al Viro <viro@ZenIV.linux.org.uk> - 2017-09-26 03:50 +0200
          Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Al Viro <viro@ZenIV.linux.org.uk> - 2017-09-26 04:10 +0200
            Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-26 04:10 +0200
              Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-26 04:10 +0200
                Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Al Viro <viro@ZenIV.linux.org.uk> - 2017-09-26 05:10 +0200
          Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-26 04:10 +0200
        Re: [git pull] vfs.git regression fix Re: Regression related to ipc  shmctl compat Christoph Hellwig <hch@infradead.org> - 2017-09-26 08:50 +0200

#1739314 — Regression related to ipc shmctl compat

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

[toc] | [next] | [standalone]

#1739414 — [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [next] | [standalone]

#1739420 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1739422 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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?

[toc] | [prev] | [next] | [standalone]

#1739432 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [next] | [standalone]

#1739436 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [next] | [standalone]

#1739445 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [next] | [standalone]

#1739469 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [next] | [standalone]

#1739438 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [next] | [standalone]

#1739554 — Re: [git pull] vfs.git regression fix Re: Regression related to ipc shmctl compat

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web