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

Groups > linux.kernel > #1296043 > unrolled thread

Re: GPF in shm_lock ipc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: GPF in shm_lock ipc Dmitry Vyukov <dvyukov@google.com> - 2015-12-21 16:50 +0100
    Re: GPF in shm_lock ipc Manfred Spraul <manfred@colorfullife.com> - 2016-01-02 12:40 +0100
      Re: GPF in shm_lock ipc Dmitry Vyukov <dvyukov@google.com> - 2016-01-02 13:30 +0100
        Re: GPF in shm_lock ipc Manfred Spraul <manfred@colorfullife.com> - 2016-01-02 17:00 +0100

#1296043 — Re: GPF in shm_lock ipc

On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> What about this:


Ping. This is still happening for me on tip. Can we pull in this fix
if it looks good to everybody?


> From 06b0fc9d62592f6f3ad9f45cccf1f6a5b3113bdc Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Thu, 5 Nov 2015 15:53:04 +0200
> Subject: [PATCH] ipc/shm: handle removed segments gracefully in shm_mmap()
>
> remap_file_pages(2) emulation can reach file which represents removed
> IPC ID as long as a memory segment is mapped. It breaks expectations
> of IPC subsystem.
>
> Test case (rewritten to be more human readable, originally autogenerated
> by syzkaller[1]):
>
>         #define _GNU_SOURCE
>         #include <stdlib.h>
>         #include <sys/ipc.h>
>         #include <sys/mman.h>
>         #include <sys/shm.h>
>
>         #define PAGE_SIZE 4096
>
>         int main()
>         {
>                 int id;
>                 void *p;
>
>                 id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
>                 p = shmat(id, NULL, 0);
>                 shmctl(id, IPC_RMID, NULL);
>                 remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
>
>                 return 0;
>         }
>
> The patch changes shm_mmap() and code around shm_lock() to propagate
> locking error back to caller of shm_mmap().
>
> [1] http://github.com/google/syzkaller
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  ipc/shm.c | 53 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 41787276e141..3174634ca4e5 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
>         struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
>
>         /*
> -        * We raced in the idr lookup or with shm_destroy().  Either way, the
> -        * ID is busted.
> +        * Callers of shm_lock() must validate the status of the returned ipc
> +        * object pointer (as returned by ipc_lock()), and error out as
> +        * appropriate.
>          */
> -       WARN_ON(IS_ERR(ipcp));
> -
> +       if (IS_ERR(ipcp))
> +               return (void *)ipcp;
>         return container_of(ipcp, struct shmid_kernel, shm_perm);
>  }
>
> @@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
>  }
>
>
> -/* This is called by fork, once for every shm attach. */
> -static void shm_open(struct vm_area_struct *vma)
> +static int __shm_open(struct vm_area_struct *vma)
>  {
>         struct file *file = vma->vm_file;
>         struct shm_file_data *sfd = shm_file_data(file);
>         struct shmid_kernel *shp;
>
>         shp = shm_lock(sfd->ns, sfd->id);
> +
> +       if (IS_ERR(shp))
> +               return PTR_ERR(shp);
> +
>         shp->shm_atim = get_seconds();
>         shp->shm_lprid = task_tgid_vnr(current);
>         shp->shm_nattch++;
>         shm_unlock(shp);
> +       return 0;
> +}
> +
> +/* This is called by fork, once for every shm attach. */
> +static void shm_open(struct vm_area_struct *vma)
> +{
> +       int err = __shm_open(vma);
> +       /*
> +        * We raced in the idr lookup or with shm_destroy().
> +        * Either way, the ID is busted.
> +        */
> +       WARN_ON_ONCE(err);
>  }
>
>  /*
> @@ -260,6 +276,14 @@ static void shm_close(struct vm_area_struct *vma)
>         down_write(&shm_ids(ns).rwsem);
>         /* remove from the list of attaches of the shm segment */
>         shp = shm_lock(ns, sfd->id);
> +
> +       /*
> +        * We raced in the idr lookup or with shm_destroy().
> +        * Either way, the ID is busted.
> +        */
> +       if (WARN_ON_ONCE(IS_ERR(shp)))
> +               goto done; /* no-op */
> +
>         shp->shm_lprid = task_tgid_vnr(current);
>         shp->shm_dtim = get_seconds();
>         shp->shm_nattch--;
> @@ -267,6 +291,7 @@ static void shm_close(struct vm_area_struct *vma)
>                 shm_destroy(ns, shp);
>         else
>                 shm_unlock(shp);
> +done:
>         up_write(&shm_ids(ns).rwsem);
>  }
>
> @@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
>         struct shm_file_data *sfd = shm_file_data(file);
>         int ret;
>
> +       /*
> +        * In case of remap_file_pages() emulation, the file can represent
> +        * removed IPC ID: propogate shm_lock() error to caller.
> +        */
> +       ret =__shm_open(vma);
> +       if (ret)
> +               return ret;
> +
>         ret = sfd->file->f_op->mmap(sfd->file, vma);
> -       if (ret != 0)
> +       if (ret) {
> +               shm_close(vma);
>                 return ret;
> +       }
>         sfd->vm_ops = vma->vm_ops;
>  #ifdef CONFIG_MMU
>         WARN_ON(!sfd->vm_ops->fault);
>  #endif
>         vma->vm_ops = &shm_vm_ops;
> -       shm_open(vma);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int shm_release(struct inode *ino, struct file *file)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300060

Hi Dmitry,

shm locking differs too much from msg/sem locking, I never looked at it 
in depth, so I'm not able to perform a proper review.

Except for the obvious: Races that can be triggered from user space are 
inacceptable.
Regardless if there is a BUG_ON, a WARN_ON or nothing at all.

On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:
>> +
>> +/* This is called by fork, once for every shm attach. */
>> +static void shm_open(struct vm_area_struct *vma)
>> +{
>> +       int err = __shm_open(vma);
>> +       /*
>> +        * We raced in the idr lookup or with shm_destroy().
>> +        * Either way, the ID is busted.
>> +        */
>> +       WARN_ON_ONCE(err);
>>   }
Is it possible to trigger this race? Parallel IPC_RMID & fork()?

--
     Manfred
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1300071

On Sat, Jan 2, 2016 at 12:33 PM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> Hi Dmitry,
>
> shm locking differs too much from msg/sem locking, I never looked at it in
> depth, so I'm not able to perform a proper review.
>
> Except for the obvious: Races that can be triggered from user space are
> inacceptable.
> Regardless if there is a BUG_ON, a WARN_ON or nothing at all.
>
> On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:
>>>
>>> +
>>> +/* This is called by fork, once for every shm attach. */
>>> +static void shm_open(struct vm_area_struct *vma)
>>> +{
>>> +       int err = __shm_open(vma);
>>> +       /*
>>> +        * We raced in the idr lookup or with shm_destroy().
>>> +        * Either way, the ID is busted.
>>> +        */
>>> +       WARN_ON_ONCE(err);
>>>   }
>
> Is it possible to trigger this race? Parallel IPC_RMID & fork()?

Hi Manfred,

As far as I see my reproducer triggers exactly this warning (and later a crash).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1300090

Hi Dmitry,

On 01/02/2016 01:19 PM, Dmitry Vyukov wrote:
> On Sat, Jan 2, 2016 at 12:33 PM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>> Hi Dmitry,
>>
>> shm locking differs too much from msg/sem locking, I never looked at it in
>> depth, so I'm not able to perform a proper review.
>>
>> Except for the obvious: Races that can be triggered from user space are
>> inacceptable.
>> Regardless if there is a BUG_ON, a WARN_ON or nothing at all.
>>
>> On 12/21/2015 04:44 PM, Dmitry Vyukov wrote:
>>>> +
>>>> +/* This is called by fork, once for every shm attach. */
>>>> +static void shm_open(struct vm_area_struct *vma)
>>>> +{
>>>> +       int err = __shm_open(vma);
>>>> +       /*
>>>> +        * We raced in the idr lookup or with shm_destroy().
>>>> +        * Either way, the ID is busted.
>>>> +        */
>>>> +       WARN_ON_ONCE(err);
>>>>    }
>> Is it possible to trigger this race? Parallel IPC_RMID & fork()?
> Hi Manfred,
>
> As far as I see my reproducer triggers exactly this warning (and later a crash).
Do I understand it right, shm_open() is also called by remap()?
Then please update the comment above shm_open().

And: If this is something that userspace can trigger, why a WARN_ON_ONCE()?
If the WARN_ON doesn't indicate a bug, then I would remove it entirely.

--
     Manfred
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web