Re: fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

From: Andrew Morton
Date: Thu Jan 27 2011 - 20:10:19 EST


Blast from the past...

On Tue, 2 Nov 2010 23:36:02 +0100
David Sterba <dsterba@xxxxxxx> wrote:

> coccinelle check scripts/coccinelle/locks/call_kern.cocci found that
> in fs/ocfs2/dlm/dlmdomain.c an allocation with GFP_KERNEL is done
> with locks held:
>
> dlm_query_region_handler
> spin_lock(dlm_domain_lock)
> dlm_match_regions
> kmalloc(GFP_KERNEL)
>
> Change it to GFP_ATOMIC.
>
> Signed-off-by: David Sterba <dsterba@xxxxxxx>
> CC: Joel Becker <joel.becker@xxxxxxxxxx>
> CC: Mark Fasheh <mfasheh@xxxxxxxx>
> CC: ocfs2-devel@xxxxxxxxxxxxxx
>
> --
> Exists in v2.6.37-rc1 and current linux-next.
>
> diff -u -p a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
> --- a/fs/ocfs2/dlm/dlmdomain.c 2010-10-22 10:23:23.502434402 +0200
> +++ b/fs/ocfs2/dlm/dlmdomain.c 2010-11-02 17:11:06.000000000 +0100
> @@ -959,7 +959,7 @@ static int dlm_match_regions(struct dlm_
> r += O2HB_MAX_REGION_NAME_LEN;
> }
>
> - local = kmalloc(sizeof(qr->qr_regions), GFP_KERNEL);
> + local = kmalloc(sizeof(qr->qr_regions), GFP_ATOMIC);
> if (!local) {
> status = -ENOMEM;
> goto bail;
>

Switching to GFP_ATOMIC is the worst of all possible ways of "fixing"
this bug. GFP_ATOMIC is *unreliable*. We don't want to introduce
unreliability deep down inside our filesytems. And fs maintainers who
don't want to make their filesystems less reliable shouldn't blindly
apply patches that do so :(


A reliable way of fixing this bug is below. As an added bonus, it
makes the fs faster.

--- a/fs/ocfs2/dlm/dlmdomain.c~a
+++ a/fs/ocfs2/dlm/dlmdomain.c
@@ -926,9 +926,9 @@ static int dlm_assert_joined_handler(str
}

static int dlm_match_regions(struct dlm_ctxt *dlm,
- struct dlm_query_region *qr)
+ struct dlm_query_region *qr, u8 *local)
{
- char *local = NULL, *remote = qr->qr_regions;
+ char *remote = qr->qr_regions;
char *l, *r;
int localnr, i, j, foundit;
int status = 0;
@@ -957,12 +957,6 @@ static int dlm_match_regions(struct dlm_
r += O2HB_MAX_REGION_NAME_LEN;
}

- local = kmalloc(sizeof(qr->qr_regions), GFP_ATOMIC);
- if (!local) {
- status = -ENOMEM;
- goto bail;
- }
-
localnr = o2hb_get_all_regions(local, O2NM_MAX_REGIONS);

/* compare local regions with remote */
@@ -1012,8 +1006,6 @@ static int dlm_match_regions(struct dlm_
}

bail:
- kfree(local);
-
return status;
}

@@ -1077,6 +1069,7 @@ static int dlm_query_region_handler(stru
struct dlm_ctxt *dlm = NULL;
int status = 0;
int locked = 0;
+ static u8 local[sizeof(qr->qr_regions)]; /* locked by dlm_domain_lock */

qr = (struct dlm_query_region *) msg->buf;

@@ -1112,7 +1105,7 @@ static int dlm_query_region_handler(stru
goto bail;
}

- status = dlm_match_regions(dlm, qr);
+ status = dlm_match_regions(dlm, qr, local);

bail:
if (locked)
_


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