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

[Mike Frysinger]

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/