Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call

[Mike Frysinger <vapier.adi@gmail.com>]

On Mon, Aug 24, 2009 at 11:30, Hugh Dickins wrote:
> --- 2.6.31-rc7/ipc/shm.c        2009-06-25 05:18:09.000000000 +0100
> +++ linux/ipc/shm.c     2009-08-24 16:06:30.000000000 +0100
> @@ -174,7 +174,7 @@ static void shm_destroy(struct ipc_names
>        shm_unlock(shp);
>        if (!is_file_hugepages(shp->shm_file))
>                shmem_lock(shp->shm_file, 0, shp->mlock_user);
> -       else
> +       else if (shp->mlock_user)
>                user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,
>                                                shp->mlock_user);
>        fput (shp->shm_file);
> @@ -369,8 +369,8 @@ static int newseg(struct ipc_namespace *
>                /* hugetlb_file_setup applies strict accounting */
>                if (shmflg & SHM_NORESERVE)
>                        acctflag = VM_NORESERVE;
> -               file = hugetlb_file_setup(name, size, acctflag);
> -               shp->mlock_user = current_user();
> +               file = hugetlb_file_setup(name, size, acctflag,
> +                                                       &shp->mlock_user);
>        } else {
>                /*
>                 * Do not allow no accounting for OVERCOMMIT_NEVER, even
> @@ -410,6 +410,8 @@ static int newseg(struct ipc_namespace *
>        return error;
>
>  no_id:
> +       if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
> +               user_shm_unlock(size, shp->mlock_user);
>        fput(file);
>  no_file:
>        security_shm_free(shp);

this breaks on no-mmu systems due to user_shm_unlock() being
mmu-specific.  normally gcc is smart enough to do dead code culling so
it hasnt caused problems, but not here.  hugetlb support is not
available on no-mmu systems, so the stubbed hugepage functions prevent
calls to user_shm_unlock() and such, but here gcc cant figure it out:

static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
{
...
    shp->mlock_user = NULL;
...
    if (shmflg & SHM_HUGETLB) {
        /* hugetlb_file_setup applies strict accounting */
        if (shmflg & SHM_NORESERVE)
            acctflag = VM_NORESERVE;
        file = hugetlb_file_setup(name, size, acctflag,
                            &shp->mlock_user);
...
    id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
    if (id < 0) {
        error = id;
        goto no_id;
    }
...
no_id:
    if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
        user_shm_unlock(size, shp->mlock_user);
...

hugetlb_file_setup() expands to nothing and so mlock_user will never
come back from NULL, but gcc still emits a reference to
user_shm_unlock() in the error path.  perhaps the best thing here is
to just add an #ifdef ?
 no_id:
+#ifdef CONFIG_HUGETLB_PAGE
+    /* gcc isn't smart enough to see that mlock_user goes non-NULL
only by hugetlb */
    if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
        user_shm_unlock(size, shp->mlock_user);
+#endif
-mike

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>