| Message ID | 20180419100935.340306831@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 19 2018 at 6:04am -0400, Thomas Gleixner <tglx@linutronix.de> wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > The allocation of the reed solomon control structure can fail, but > fec_alloc_bufs() ignores that and subsequent operations in dm verity use > the potential NULL pointer unconditionally. > > Add a proper check and abort if init_rs() fails. This changelog makes little sense: init_rs() isn't in play relative to this patch. And it runs counter to this commit's changelog: commit 34c96507e8f6be497c15497be05f489fb34c5880 Author: NeilBrown <neilb@suse.com> Date: Mon Apr 10 12:13:00 2017 +1000 dm verity fec: fix GFP flags used with mempool_alloc() mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no point testing for failure. One place the code tested for failure was passing "0" as the GFP flags. This is most unusual and is probably meant to be GFP_NOIO, so that is changed. Also, allocation from ->extra_pool and ->prealloc_pool are repeated before releasing the previous allocation. This can deadlock if the code is servicing a write under high memory pressure. To avoid deadlocks, change these to use GFP_NOWAIT and leave the error handling in place. Signed-off-by: NeilBrown <neilb@suse.com> Signed-off-by: Mike Snitzer <snitzer@redhat.com> Seems there is no real need for this patch. Neil, what do you think? Thanks, Mike > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Segher Boessenkool <segher@kernel.crashing.org> > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Mike Snitzer <snitzer@redhat.com> > Cc: Anton Vorontsov <anton@enomsg.org> > Cc: Colin Cross <ccross@android.com> > Cc: Andrew Morton <akpm@linuxfoundation.org> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Alasdair Kergon <agk@redhat.com> > > --- > drivers/md/dm-verity-fec.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > --- a/drivers/md/dm-verity-fec.c > +++ b/drivers/md/dm-verity-fec.c > @@ -308,8 +308,13 @@ static int fec_alloc_bufs(struct dm_veri > { > unsigned n; > > - if (!fio->rs) > + if (!fio->rs) { > fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO); > + if (!fio->rs) { > + DMERR("failed to allocate RS control structure"); > + return -ENOMEM; > + } > + } > > fec_for_each_prealloc_buffer(n) { > if (fio->bufs[n]) > > > > >
On Thu, 19 Apr 2018, Mike Snitzer wrote: > On Thu, Apr 19 2018 at 6:04am -0400, > Thomas Gleixner <tglx@linutronix.de> wrote: > > > From: Thomas Gleixner <tglx@linutronix.de> > > > > The allocation of the reed solomon control structure can fail, but > > fec_alloc_bufs() ignores that and subsequent operations in dm verity use > > the potential NULL pointer unconditionally. > > > > Add a proper check and abort if init_rs() fails. > > This changelog makes little sense: init_rs() isn't in play relative to > this patch. fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO); f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc, fec_rs_free, (void *) v); static void *fec_rs_alloc(gfp_t gfp_mask, void *pool_data) { struct dm_verity *v = (struct dm_verity *)pool_data; return init_rs(8, 0x11d, 0, 1, v->fec->roots); } So init_rs() is part of the chain, right? Yes. I missed the NOIO part. But.... > And it runs counter to this commit's changelog: > > commit 34c96507e8f6be497c15497be05f489fb34c5880 > Author: NeilBrown <neilb@suse.com> > Date: Mon Apr 10 12:13:00 2017 +1000 > > dm verity fec: fix GFP flags used with mempool_alloc() > > mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no > point testing for failure. > > One place the code tested for failure was passing "0" as the GFP > flags. This is most unusual and is probably meant to be GFP_NOIO, > so that is changed. > > Also, allocation from ->extra_pool and ->prealloc_pool are repeated > before releasing the previous allocation. This can deadlock if the code > is servicing a write under high memory pressure. To avoid deadlocks, > change these to use GFP_NOWAIT and leave the error handling in place. > > Signed-off-by: NeilBrown <neilb@suse.com> > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > > Seems there is no real need for this patch. Neil, what do you think? 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. Thanks, tglx
On Thu, Apr 19 2018, Thomas Gleixner wrote: > On Thu, 19 Apr 2018, Mike Snitzer wrote: > >> On Thu, Apr 19 2018 at 6:04am -0400, >> Thomas Gleixner <tglx@linutronix.de> wrote: >> >> > From: Thomas Gleixner <tglx@linutronix.de> >> > >> > The allocation of the reed solomon control structure can fail, but >> > fec_alloc_bufs() ignores that and subsequent operations in dm verity use >> > the potential NULL pointer unconditionally. >> > >> > Add a proper check and abort if init_rs() fails. >> >> This changelog makes little sense: init_rs() isn't in play relative to >> this patch. > > fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO); > > f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc, > fec_rs_free, (void *) v); > > static void *fec_rs_alloc(gfp_t gfp_mask, void *pool_data) > { > struct dm_verity *v = (struct dm_verity *)pool_data; > > return init_rs(8, 0x11d, 0, 1, v->fec->roots); > } > > So init_rs() is part of the chain, right? > > Yes. I missed the NOIO part. But.... > >> And it runs counter to this commit's changelog: >> >> commit 34c96507e8f6be497c15497be05f489fb34c5880 >> Author: NeilBrown <neilb@suse.com> >> Date: Mon Apr 10 12:13:00 2017 +1000 >> >> dm verity fec: fix GFP flags used with mempool_alloc() >> >> mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no >> point testing for failure. >> >> One place the code tested for failure was passing "0" as the GFP >> flags. This is most unusual and is probably meant to be GFP_NOIO, >> so that is changed. >> >> Also, allocation from ->extra_pool and ->prealloc_pool are repeated >> before releasing the previous allocation. This can deadlock if the code >> is servicing a write under high memory pressure. To avoid deadlocks, >> change these to use GFP_NOWAIT and leave the error handling in place. >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> Signed-off-by: Mike Snitzer <snitzer@redhat.com> >> >> Seems there is no real need for this patch. Neil, what do you think? I think the code is correct as-is. > > 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. 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. 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(). Thanks, NeilBrown
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
--- a/drivers/md/dm-verity-fec.c +++ b/drivers/md/dm-verity-fec.c @@ -308,8 +308,13 @@ static int fec_alloc_bufs(struct dm_veri { unsigned n; - if (!fio->rs) + if (!fio->rs) { fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO); + if (!fio->rs) { + DMERR("failed to allocate RS control structure"); + return -ENOMEM; + } + } fec_for_each_prealloc_buffer(n) { if (fio->bufs[n])