[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.21.1804201040420.1683@nanos.tec.linutronix.de>
Date: Fri, 20 Apr 2018 10:49:05 +0200 (CEST)
From: Thomas Gleixner <tglx@...utronix.de>
To: NeilBrown <neilb@...e.com>
cc: Mike Snitzer <snitzer@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
Kees Cook <keescook@...omium.org>,
Segher Boessenkool <segher@...nel.crashing.org>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
Andrew Morton <akpm@...uxfoundation.org>,
Boris Brezillon <boris.brezillon@...e-electrons.com>,
Richard Weinberger <richard@....at>, David Woodhouse <dwmw2@...radead.org>,
Alasdair Kergon <agk@...hat.com>, Anton Vorontsov <anton@...msg.org>,
Colin Cross <ccross@...roid.com>, Tony Luck <tony.luck@...el.com>
Subject: Re: [patch V2 7/8] dm verity fec: Check result of init_rs()
On Fri, 20 Apr 2018, NeilBrown wrote:
> On Thu, Apr 19 2018, Thomas Gleixner wrote:
> > The analysis above forgot to look at the mempool->alloc() callback. So yes,
> > while the NOIO is good at the mempool level, but init_rs() uses GPF_KERNEL
> > so there might be a different can of wurms lurking.
>
> The ->alloc call back is not relevant to the question of when
> mempool_alloc() can return NULL.
> If the ->alloc() callback returns a non-NULL value, it will be returned
> by mempool_alloc().
> If it returns NULL, that will not be returned.
Yes, as I said before, I missed the NOIO flag.
>
> mempool_alloc() *only* returns NULL in one place:
>
> if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
> spin_unlock_irqrestore(&pool->lock, flags);
> return NULL;
> }
>
> so a NULL return is purely dependent on the GFP flags passed.
> GFP_NOIO contains __GFP_DIRECT_RECLAIM, so NULL cannot be returned.
>
> It seems quite broken that init_rs() uses GFP_KERNEL. It should take a
> gfp_t arg for the allocation.
Well, init_rs() was that way before somebody used it in a mempool_alloc()
callback. And all other users are fine with GFP_KERNEL AFAICT.
> If the mempool_alloc() above really needs GFP_NOIO, then it could
> theoretically deadlock as it performs a GFP_KERNEL allocation inside
> rs_init(). So in that sense, the code is not correct as-is.
> It could possibly be fixed by calling memalloc_noio_save() /
> memalloc_noio_restore() around the call to init_rs() in fec_rs_alloc().
No, we surely can add a gfp aware version of init_rs(). It's trivial enough
to do.
Thanks,
tglx
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
