[PATCH v2 0/3] a few cleanup and bugfixes about shmem

[PATCH v2 0/3] a few cleanup and bugfixes about shmem

[Chen Wandun]

v1 ==> v2:
combine patch2 and patch3 into a single patch.

Chen Wandun (3):
  mm/shmem: check return value of shmem_init_inodecache
  mm/shmem: return error code directly for invalid addr
  mm/shmem: rework calculation of inflated_addr in
    shmem_get_unmapped_area

 mm/shmem.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] mm/shmem: check return value of shmem_init_inodecache

[Chen Wandun]

It will result in null pointer access if shmem_init_inodecache fail,
so check return value of shmem_init_inodecache

Signed-off-by: Chen Wandun <chenwandun@huawei.com>
---
 mm/shmem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 12d45a03f7fc..17f8297ece29 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3775,11 +3775,13 @@ static void shmem_init_inode(void *foo)
 	inode_init_once(&info->vfs_inode);
 }
 
-static void shmem_init_inodecache(void)
+static struct kmem_cache *shmem_init_inodecache(void)
 {
 	shmem_inode_cachep = kmem_cache_create("shmem_inode_cache",
 				sizeof(struct shmem_inode_info),
 				0, SLAB_PANIC|SLAB_ACCOUNT, shmem_init_inode);
+
+	return shmem_inode_cachep;
 }
 
 static void shmem_destroy_inodecache(void)
@@ -3923,7 +3925,10 @@ void __init shmem_init(void)
 {
 	int error;
 
-	shmem_init_inodecache();
+	if (!shmem_init_inodecache()) {
+		error = -ENOMEM;
+		goto out2;
+	}
 
 	error = register_filesystem(&shmem_fs_type);
 	if (error) {
-- 
2.25.1

[PATCH v2 2/3] mm/shmem: return error code directly for invalid addr

[Chen Wandun]

Return error code directly for addr is not PAGE_SIZE aligned or
beyond TASK_SIZE, no need to check these cases in caller.

Signed-off-by: Chen Wandun <chenwandun@huawei.com>
---
 mm/shmem.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 17f8297ece29..5e61692f2d8e 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2142,10 +2142,10 @@ unsigned long shmem_get_unmapped_area(struct file *file,
 		return addr;
 	if (IS_ERR_VALUE(addr))
 		return addr;
-	if (addr & ~PAGE_MASK)
-		return addr;
+	if (offset_in_page(addr))
+		return -EINVAL;
 	if (addr > TASK_SIZE - len)
-		return addr;
+		return -ENOMEM;
 
 	if (shmem_huge == SHMEM_HUGE_DENY)
 		return addr;
@@ -2196,7 +2196,7 @@ unsigned long shmem_get_unmapped_area(struct file *file,
 	inflated_addr = get_area(NULL, uaddr, inflated_len, 0, flags);
 	if (IS_ERR_VALUE(inflated_addr))
 		return addr;
-	if (inflated_addr & ~PAGE_MASK)
+	if (offset_in_page(inflated_addr))
 		return addr;
 
 	inflated_offset = inflated_addr & (HPAGE_PMD_SIZE-1);
-- 
2.25.1

[PATCH v2 3/3] mm/shmem: rework calculation of inflated_addr in shmem_get_unmapped_area

[Chen Wandun]

In function shmem_get_unmapped_area, inflated_offset and offset
are unsigned long, it will result in underflow when offset below
inflated_offset, a little confusing, no functional change.

Signed-off-by: Chen Wandun <chenwandun@huawei.com>
---
 mm/shmem.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 5e61692f2d8e..0b6c61a7cbb3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2200,9 +2200,12 @@ unsigned long shmem_get_unmapped_area(struct file *file,
 		return addr;
 
 	inflated_offset = inflated_addr & (HPAGE_PMD_SIZE-1);
-	inflated_addr += offset - inflated_offset;
-	if (inflated_offset > offset)
+	if (offset > inflated_offset)
+		inflated_addr += offset - inflated_offset;
+	else if (offset < inflated_offset) {
+		inflated_addr -= inflated_offset - offset;
 		inflated_addr += HPAGE_PMD_SIZE;
+	}
 
 	if (inflated_addr > TASK_SIZE - len)
 		return addr;
-- 
2.25.1

Re: [PATCH v2 1/3] mm/shmem: check return value of shmem_init_inodecache

[Matthew Wilcox]

On Sun, Jun 05, 2022 at 11:55:55AM +0800, Chen Wandun wrote:
> It will result in null pointer access if shmem_init_inodecache fail,
> so check return value of shmem_init_inodecache

You ignored my suggestion from v1.  Here, let me write it out for you.

+static int shmem_init_inodecache(void)
 {
  	shmem_inode_cachep = kmem_cache_create("shmem_inode_cache",
  				sizeof(struct shmem_inode_info),
  				0, SLAB_PANIC|SLAB_ACCOUNT, shmem_init_inode);
+	if (!shmem_inode_cachep)
+		return -ENOMEM;
+	return 0;
 }

...

+	error = shmem_init_inodecache();
+	if (error)
+		goto out2;

Re: [PATCH v2 1/3] mm/shmem: check return value of shmem_init_inodecache

[Chen Wandun]

On 2022/6/5 19:49, Matthew Wilcox wrote:
> On Sun, Jun 05, 2022 at 11:55:55AM +0800, Chen Wandun wrote:
>> It will result in null pointer access if shmem_init_inodecache fail,
>> so check return value of shmem_init_inodecache
> You ignored my suggestion from v1.  Here, let me write it out for you.
Hi Matthew,
I didn't ignore your suggestion,  some explanation is needed, sorry for 
that.

In V1, Kefeng point:
"kmem_cache_create return a pointer to the cache on success, NULL on 
failure,
so error = -ENOMEM; is right :)"

so, I look some similar code such as init_inodecache in kinds of file 
system,  they all
return -ENOMEM on failure, so is it OK to return -ENOMEM on failure :)

Besides,  kmem_cache_create return NULL on failure, maybe returning 
error code
on failure is more proper, but it is another job.
>
> +static int shmem_init_inodecache(void)
>   {
>    	shmem_inode_cachep = kmem_cache_create("shmem_inode_cache",
>    				sizeof(struct shmem_inode_info),
>    				0, SLAB_PANIC|SLAB_ACCOUNT, shmem_init_inode);
> +	if (!shmem_inode_cachep)
> +		return -ENOMEM;
> +	return 0;
>   }
>
> ...
>
> +	error = shmem_init_inodecache();
> +	if (error)
> +		goto out2;
>
>
> .

Re: [PATCH v2 1/3] mm/shmem: check return value of shmem_init_inodecache

[Matthew Wilcox]

On Mon, Jun 06, 2022 at 09:34:13AM +0800, Chen Wandun wrote:
> On 2022/6/5 19:49, Matthew Wilcox wrote:
> > On Sun, Jun 05, 2022 at 11:55:55AM +0800, Chen Wandun wrote:
> > > It will result in null pointer access if shmem_init_inodecache fail,
> > > so check return value of shmem_init_inodecache
> > You ignored my suggestion from v1.  Here, let me write it out for you.
> Hi Matthew,
> I didn't ignore your suggestion,  some explanation is needed, sorry for
> that.
> 
> In V1, Kefeng point:
> "kmem_cache_create return a pointer to the cache on success, NULL on
> failure,
> so error = -ENOMEM; is right :)"
> 
> so, I look some similar code such as init_inodecache in kinds of file
> system,  they all
> return -ENOMEM on failure, so is it OK to return -ENOMEM on failure :)
> 
> Besides,  kmem_cache_create return NULL on failure, maybe returning error
> code
> on failure is more proper, but it is another job.

I literally wrote out what I think you should do instead.  Stop arguing.

> > +static int shmem_init_inodecache(void)
> >   {
> >    	shmem_inode_cachep = kmem_cache_create("shmem_inode_cache",
> >    				sizeof(struct shmem_inode_info),
> >    				0, SLAB_PANIC|SLAB_ACCOUNT, shmem_init_inode);
> > +	if (!shmem_inode_cachep)
> > +		return -ENOMEM;
> > +	return 0;
> >   }
> > 
> > ...
> > 
> > +	error = shmem_init_inodecache();
> > +	if (error)
> > +		goto out2;
> > 
> > 
> > .
> 

Re: [PATCH v2 1/3] mm/shmem: check return value of shmem_init_inodecache

[Chen Wandun]

在 2022/6/6 10:08, Matthew Wilcox 写道:
> On Mon, Jun 06, 2022 at 09:34:13AM +0800, Chen Wandun wrote:
>> On 2022/6/5 19:49, Matthew Wilcox wrote:
>>> On Sun, Jun 05, 2022 at 11:55:55AM +0800, Chen Wandun wrote:
>>>> It will result in null pointer access if shmem_init_inodecache fail,
>>>> so check return value of shmem_init_inodecache
>>> You ignored my suggestion from v1.  Here, let me write it out for you.
>> Hi Matthew,
>> I didn't ignore your suggestion,  some explanation is needed, sorry for
>> that.
>>
>> In V1, Kefeng point:
>> "kmem_cache_create return a pointer to the cache on success, NULL on
>> failure,
>> so error = -ENOMEM; is right :)"
>>
>> so, I look some similar code such as init_inodecache in kinds of file
>> system,  they all
>> return -ENOMEM on failure, so is it OK to return -ENOMEM on failure :)
>>
>> Besides,  kmem_cache_create return NULL on failure, maybe returning error
>> code
>> on failure is more proper, but it is another job.
> I literally wrote out what I think you should do instead.  Stop arguing.
>
>>> +static int shmem_init_inodecache(void)
>>>    {
>>>     	shmem_inode_cachep = kmem_cache_create("shmem_inode_cache",
>>>     				sizeof(struct shmem_inode_info),
>>>     				0, SLAB_PANIC|SLAB_ACCOUNT, shmem_init_inode);
>>> +	if (!shmem_inode_cachep)
>>> +		return -ENOMEM;
>>> +	return 0;
>>>    }
>>>
>>> ...
>>>
>>> +	error = shmem_init_inodecache();
>>> +	if (error)
>>> +		goto out2;
Oh, I misunderstood what you said, feel so sorry.
I will send a new version.

Thanks.
>>>
>>>
>>> .
> .