Re: [PATCH linux-next] lockd: nlmsvc_mark_resources(): avoid stackoverflow

From: J. Bruce Fields
Date: Tue Feb 12 2013 - 16:22:54 EST


On Tue, Feb 12, 2013 at 12:48:58PM -0700, Tim Gardner wrote:
> Dynamically allocate the NLM host structure in order to avoid stack overflow.
> nlmsvc_mark_resources() is several call levels deep in a stack
> that has a number of large variables. 512 bytes seems like a lot
> on the stack at this point.
>
> smatch analysis:
>
> fs/lockd/svcsubs.c:366 nlmsvc_mark_resources() warn: 'hint' puts
> 512 bytes on stack
>
> Cc: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> Cc: "J. Bruce Fields" <bfields@xxxxxxxxxxxx>
> Cc: linux-nfs@xxxxxxxxxxxxxxx
> Signed-off-by: Tim Gardner <tim.gardner@xxxxxxxxxxxxx>
> ---
> fs/lockd/svcsubs.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index b904f41..f3abb7f 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -363,11 +363,15 @@ nlmsvc_is_client(void *data, struct nlm_host *dummy)
> void
> nlmsvc_mark_resources(struct net *net)
> {
> - struct nlm_host hint;
> + struct nlm_host *hint = kzalloc(sizeof(*hint), GFP_KERNEL);
>
> - dprintk("lockd: nlmsvc_mark_resources for net %p\n", net);
> - hint.net = net;
> - nlm_traverse_files(&hint, nlmsvc_mark_host, NULL);
> + if (hint) {
> + dprintk("lockd: nlmsvc_mark_resources for net %p\n", net);
> + hint->net = net;
> + nlm_traverse_files(hint, nlmsvc_mark_host, NULL);
> + }

Silently neglecting to do this looks like a bad idea.

It's strange that we're passing in an nlm_host when all we actually use
is the struct net*. Why not just change this to pass in the net
instead?

--b.

> +
> + kfree(hint);
> }
>
> /*
> --
> 1.7.9.5
>
--
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/