NFS: check for allocation failure from mempool_alloc
diff mbox series

Message ID 20200226234320.7722-1-colin.king@canonical.com
State New
Headers show
Series
  • NFS: check for allocation failure from mempool_alloc
Related show

Commit Message

Colin King Feb. 26, 2020, 11:43 p.m. UTC
From: Colin Ian King <colin.king@canonical.com>

It is possible for mempool_alloc to return null when using
the GFP_KERNEL flag, so return NULL and avoid a null pointer
dereference on the following memset of the null pointer.

Addresses-Coverity: ("Dereference null return")
Fixes: 2b17d725f9be ("NFS: Clean up writeback code")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 fs/nfs/write.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Trond Myklebust Feb. 26, 2020, 11:48 p.m. UTC | #1
On Wed, 2020-02-26 at 23:43 +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> It is possible for mempool_alloc to return null when using
> the GFP_KERNEL flag, so return NULL and avoid a null pointer
> dereference on the following memset of the null pointer.

Umm, no. That would be a false positive by coverity.

If you look at the history of that function, you'll note that we
originally had those checks, but that Neil Brown removed them after
analysis of the mempool_alloc() function. He determined (correctly, I
believe) that any value that includes GFP_WAIT cannot fail to return a
valid pointer.

> 
> Addresses-Coverity: ("Dereference null return")
> Fixes: 2b17d725f9be ("NFS: Clean up writeback code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/nfs/write.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index c478b772cc49..7ca036660dd1 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -106,6 +106,9 @@ static struct nfs_pgio_header
> *nfs_writehdr_alloc(void)
>  {
>  	struct nfs_pgio_header *p = mempool_alloc(nfs_wdata_mempool,
> GFP_KERNEL);
>  
> +	if (!p)
> +		return NULL;
> +
>  	memset(p, 0, sizeof(*p));
>  	p->rw_mode = FMODE_WRITE;
>  	return p;
Colin King Feb. 26, 2020, 11:56 p.m. UTC | #2
On 26/02/2020 23:48, Trond Myklebust wrote:
> On Wed, 2020-02-26 at 23:43 +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> It is possible for mempool_alloc to return null when using
>> the GFP_KERNEL flag, so return NULL and avoid a null pointer
>> dereference on the following memset of the null pointer.
> 
> Umm, no. That would be a false positive by coverity.

Ah, sorry for the noise then.

> 
> If you look at the history of that function, you'll note that we
> originally had those checks, but that Neil Brown removed them after
> analysis of the mempool_alloc() function. He determined (correctly, I
> believe) that any value that includes GFP_WAIT cannot fail to return a
> valid pointer.

OK - that's very helpful to know. That allows me to mark a shed load of
false positives on mempool_alloc false positives.

Colin

> 
>>
>> Addresses-Coverity: ("Dereference null return")
>> Fixes: 2b17d725f9be ("NFS: Clean up writeback code")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  fs/nfs/write.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index c478b772cc49..7ca036660dd1 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -106,6 +106,9 @@ static struct nfs_pgio_header
>> *nfs_writehdr_alloc(void)
>>  {
>>  	struct nfs_pgio_header *p = mempool_alloc(nfs_wdata_mempool,
>> GFP_KERNEL);
>>  
>> +	if (!p)
>> +		return NULL;
>> +
>>  	memset(p, 0, sizeof(*p));
>>  	p->rw_mode = FMODE_WRITE;
>>  	return p;
Dan Carpenter March 2, 2020, 7:30 a.m. UTC | #3
On Wed, Feb 26, 2020 at 11:43:20PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> It is possible for mempool_alloc to return null when using
> the GFP_KERNEL flag, so return NULL and avoid a null pointer
> dereference on the following memset of the null pointer.
> 
> Addresses-Coverity: ("Dereference null return")
> Fixes: 2b17d725f9be ("NFS: Clean up writeback code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/nfs/write.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index c478b772cc49..7ca036660dd1 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -106,6 +106,9 @@ static struct nfs_pgio_header *nfs_writehdr_alloc(void)
>  {
>  	struct nfs_pgio_header *p = mempool_alloc(nfs_wdata_mempool, GFP_KERNEL);
>  
> +	if (!p)

The fixes tag was wrong.  When I searched for the correct fixes tag,
it turned out this was intentional.  See commit 237f8306c302
("NFS: don't expect errors from mempool_alloc().") and commit 518662e0fcb9
("NFS: fix usage of mempools.").

    When passed GFP flags that allow sleeping (such as
    GFP_NOIO), mempool_alloc() will never return NULL, it will
    wait until memory is available.

regards,
dan carpenter

Patch
diff mbox series

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index c478b772cc49..7ca036660dd1 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -106,6 +106,9 @@  static struct nfs_pgio_header *nfs_writehdr_alloc(void)
 {
 	struct nfs_pgio_header *p = mempool_alloc(nfs_wdata_mempool, GFP_KERNEL);
 
+	if (!p)
+		return NULL;
+
 	memset(p, 0, sizeof(*p));
 	p->rw_mode = FMODE_WRITE;
 	return p;