From: Thomas Gleixner <[email protected]>
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.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Segher Boessenkool <[email protected]>
Cc: Kernel Hardening <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Mike Snitzer <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Colin Cross <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Alasdair Kergon <[email protected]>
---
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, Apr 19 2018 at 6:04am -0400,
Thomas Gleixner <[email protected]> wrote:
> From: Thomas Gleixner <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mike Snitzer <[email protected]>
Seems there is no real need for this patch. Neil, what do you think?
Thanks,
Mike
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Segher Boessenkool <[email protected]>
> Cc: Kernel Hardening <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Mike Snitzer <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Colin Cross <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: David Woodhouse <[email protected]>
> Cc: Alasdair Kergon <[email protected]>
>
> ---
> 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 <[email protected]> wrote:
>
> > From: Thomas Gleixner <[email protected]>
> >
> > 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Mike Snitzer <[email protected]>
>
> 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 <[email protected]> wrote:
>>
>> > From: Thomas Gleixner <[email protected]>
>> >
>> > 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 <[email protected]>
>> 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 <[email protected]>
>> Signed-off-by: Mike Snitzer <[email protected]>
>>
>> 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