[PATCH] fix proc_reg_get_unmapped_area()

[PATCH] fix proc_reg_get_unmapped_area()

[Jan Beulich]

Commit c4fe24485729fc2cbff324c111e67a1cc2f9adea ("sparc: fix PCI device
proc file mmap(2)"), while fixing one problem, introduced two new ones:
- the function truncates the return value from ->get_unmapped_area() on
  64-bit architectures,
- _all_ descendants are now required to set .get_unmapped_area to non-
  NULL, which wasn't necessary before (and shouldn't be).

Both - afaict - are a result from a too simplistic copy'n'paste from
proc_reg_mmap() done in that change.

This likely also addresses reports like the one at
http://linux-kernel.2935.n7.nabble.com/mmap-for-proc-vmcore-broken-since-3-12-rc1-td729326.html.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>

---
 fs/proc/inode.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- 3.12-rc5/fs/proc/inode.c
+++ 3.12-rc5-proc-get-unmapped-area/fs/proc/inode.c
@@ -288,12 +288,12 @@ static int proc_reg_mmap(struct file *fi
 static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned long orig_addr, unsigned long len, unsigned long pgoff, unsigned long flags)
 {
 	struct proc_dir_entry *pde = PDE(file_inode(file));
-	int rv = -EIO;
-	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
+	unsigned long rv = -EIO;
+
 	if (use_pde(pde)) {
-		get_unmapped_area = pde->proc_fops->get_unmapped_area;
-		if (get_unmapped_area)
-			rv = get_unmapped_area(file, orig_addr, len, pgoff, flags);
+		rv = (pde->proc_fops->get_unmapped_area
+		      ?: current->mm->get_unmapped_area)(file, orig_addr, len,
+							 pgoff, flags);
 		unuse_pde(pde);
 	}
 	return rv;

Re: [PATCH] fix proc_reg_get_unmapped_area()

[Andrew Morton]

On Wed, 16 Oct 2013 12:32:40 +0100 "Jan Beulich" <JBeulich@suse.com> wrote:

> Commit c4fe24485729fc2cbff324c111e67a1cc2f9adea ("sparc: fix PCI device
> proc file mmap(2)"), while fixing one problem, introduced two new ones:
> - the function truncates the return value from ->get_unmapped_area() on
>   64-bit architectures,
> - _all_ descendants are now required to set .get_unmapped_area to non-
>   NULL, which wasn't necessary before (and shouldn't be).
> 
> Both - afaict - are a result from a too simplistic copy'n'paste from
> proc_reg_mmap() done in that change.
> 
> This likely also addresses reports like the one at
> http://linux-kernel.2935.n7.nabble.com/mmap-for-proc-vmcore-broken-since-3-12-rc1-td729326.html.
> 
> ...
>
> --- 3.12-rc5/fs/proc/inode.c
> +++ 3.12-rc5-proc-get-unmapped-area/fs/proc/inode.c
> @@ -288,12 +288,12 @@ static int proc_reg_mmap(struct file *fi
>  static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned long orig_addr, unsigned long len, unsigned long pgoff, unsigned long flags)
>  {
>  	struct proc_dir_entry *pde = PDE(file_inode(file));
> -	int rv = -EIO;
> -	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
> +	unsigned long rv = -EIO;
> +
>  	if (use_pde(pde)) {
> -		get_unmapped_area = pde->proc_fops->get_unmapped_area;
> -		if (get_unmapped_area)
> -			rv = get_unmapped_area(file, orig_addr, len, pgoff, flags);
> +		rv = (pde->proc_fops->get_unmapped_area
> +		      ?: current->mm->get_unmapped_area)(file, orig_addr, len,
> +							 pgoff, flags);
>  		unuse_pde(pde);
>  	}
>  	return rv;

I think these two patches will address the problems:

http://ozlabs.org/~akpm/mmots/broken-out/procfs-fix-unintended-truncation-of-returned-mapped-address.patch
http://ozlabs.org/~akpm/mmots/broken-out/procfs-call-default-get_unmapped_area-on-mmu-present-architectures.patch

I'll be sending those Linuswards today.  Please check them.  (I think
your version would break the nommu build).

Re: [PATCH] fix proc_reg_get_unmapped_area()

[Jan Beulich]

>>> On 16.10.13 at 22:40, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 16 Oct 2013 12:32:40 +0100 "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> Commit c4fe24485729fc2cbff324c111e67a1cc2f9adea ("sparc: fix PCI device
>> proc file mmap(2)"), while fixing one problem, introduced two new ones:
>> - the function truncates the return value from ->get_unmapped_area() on
>>   64-bit architectures,
>> - _all_ descendants are now required to set .get_unmapped_area to non-
>>   NULL, which wasn't necessary before (and shouldn't be).
>> 
>> Both - afaict - are a result from a too simplistic copy'n'paste from
>> proc_reg_mmap() done in that change.
>> 
>> This likely also addresses reports like the one at
>> 
> http://linux-kernel.2935.n7.nabble.com/mmap-for-proc-vmcore-broken-since-3-12-rc1-td729 
> 326.html.
>> 
>> ...
>>
>> --- 3.12-rc5/fs/proc/inode.c
>> +++ 3.12-rc5-proc-get-unmapped-area/fs/proc/inode.c
>> @@ -288,12 +288,12 @@ static int proc_reg_mmap(struct file *fi
>>  static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned 
> long orig_addr, unsigned long len, unsigned long pgoff, unsigned long flags)
>>  {
>>  	struct proc_dir_entry *pde = PDE(file_inode(file));
>> -	int rv = -EIO;
>> -	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned 
> long, unsigned long, unsigned long);
>> +	unsigned long rv = -EIO;
>> +
>>  	if (use_pde(pde)) {
>> -		get_unmapped_area = pde->proc_fops->get_unmapped_area;
>> -		if (get_unmapped_area)
>> -			rv = get_unmapped_area(file, orig_addr, len, pgoff, flags);
>> +		rv = (pde->proc_fops->get_unmapped_area
>> +		      ?: current->mm->get_unmapped_area)(file, orig_addr, len,
>> +							 pgoff, flags);
>>  		unuse_pde(pde);
>>  	}
>>  	return rv;
> 
> I think these two patches will address the problems:
> 
> http://ozlabs.org/~akpm/mmots/broken-out/procfs-fix-unintended-truncation-of-retur 
> ned-mapped-address.patch
> http://ozlabs.org/~akpm/mmots/broken-out/procfs-call-default-get_unmapped_area-on- 
> mmu-present-architectures.patch
> 
> I'll be sending those Linuswards today.  Please check them.  (I think
> your version would break the nommu build).

Yes indeed - I did search for existing patches, but didn't find them.
I see Linus merged them already, so there's no point anymore
sending Reviewed-by tags.

I think though that in the !MMU case the second of them leaves an
issue unfixed nevertheless: If the specific procfs handler has no
.get_unmapped_area, the operation would still fail in that case
rather than succeed as it did before that wrapper got added.)

Jan

Re: [PATCH] fix proc_reg_get_unmapped_area()

[HATAYAMA Daisuke]

(2013/10/17 16:23), Jan Beulich wrote:
>>>> On 16.10.13 at 22:40, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Wed, 16 Oct 2013 12:32:40 +0100 "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>> Commit c4fe24485729fc2cbff324c111e67a1cc2f9adea ("sparc: fix PCI device
>>> proc file mmap(2)"), while fixing one problem, introduced two new ones:
>>> - the function truncates the return value from ->get_unmapped_area() on
>>>    64-bit architectures,
>>> - _all_ descendants are now required to set .get_unmapped_area to non-
>>>    NULL, which wasn't necessary before (and shouldn't be).
>>>
>>> Both - afaict - are a result from a too simplistic copy'n'paste from
>>> proc_reg_mmap() done in that change.
>>>
>>> This likely also addresses reports like the one at
>>>
>> http://linux-kernel.2935.n7.nabble.com/mmap-for-proc-vmcore-broken-since-3-12-rc1-td729
>> 326.html.
>>>
>>> ...
>>>
>>> --- 3.12-rc5/fs/proc/inode.c
>>> +++ 3.12-rc5-proc-get-unmapped-area/fs/proc/inode.c
>>> @@ -288,12 +288,12 @@ static int proc_reg_mmap(struct file *fi
>>>   static unsigned long proc_reg_get_unmapped_area(struct file *file, unsigned
>> long orig_addr, unsigned long len, unsigned long pgoff, unsigned long flags)
>>>   {
>>>   	struct proc_dir_entry *pde = PDE(file_inode(file));
>>> -	int rv = -EIO;
>>> -	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned
>> long, unsigned long, unsigned long);
>>> +	unsigned long rv = -EIO;
>>> +
>>>   	if (use_pde(pde)) {
>>> -		get_unmapped_area = pde->proc_fops->get_unmapped_area;
>>> -		if (get_unmapped_area)
>>> -			rv = get_unmapped_area(file, orig_addr, len, pgoff, flags);
>>> +		rv = (pde->proc_fops->get_unmapped_area
>>> +		      ?: current->mm->get_unmapped_area)(file, orig_addr, len,
>>> +							 pgoff, flags);
>>>   		unuse_pde(pde);
>>>   	}
>>>   	return rv;
>>
>> I think these two patches will address the problems:
>>
>> http://ozlabs.org/~akpm/mmots/broken-out/procfs-fix-unintended-truncation-of-retur
>> ned-mapped-address.patch
>> http://ozlabs.org/~akpm/mmots/broken-out/procfs-call-default-get_unmapped_area-on-
>> mmu-present-architectures.patch
>>
>> I'll be sending those Linuswards today.  Please check them.  (I think
>> your version would break the nommu build).
>
> Yes indeed - I did search for existing patches, but didn't find them.
> I see Linus merged them already, so there's no point anymore
> sending Reviewed-by tags.
>
> I think though that in the !MMU case the second of them leaves an
> issue unfixed nevertheless: If the specific procfs handler has no
> .get_unmapped_area, the operation would still fail in that case
> rather than succeed as it did before that wrapper got added.)
>
> Jan
>

Yes, there remains difference on no-mmu that if mmap is not defined, mmap() now returns
EIO, it should return ENODEV.

In mm/nommu.c,

                 /* eliminate any capabilities that we can't support on this
                  * device */
                 if (!file->f_op->get_unmapped_area)
                         capabilities &= ~BDI_CAP_MAP_DIRECT;

and

                  */
                 if (capabilities & BDI_CAP_MAP_DIRECT) {
                         addr = file->f_op->get_unmapped_area(file, addr, len,
                                                              pgoff, flags);
                         if (IS_ERR_VALUE(addr)) {
                                 ret = addr;
                                 if (ret != -ENOSYS)
                                         goto error_just_free;

                                 /* the driver refused to tell us where to site
                                  * the mapping so we'll have to attempt to copy
                                  * it */
                                 ret = -ENODEV;
                                 if (!(capabilities & BDI_CAP_MAP_COPY))
                                         goto error_just_free;

                                 capabilities &= ~BDI_CAP_MAP_DIRECT;
                         } else {
                                 vma->vm_start = region->vm_start = addr;
                                 vma->vm_end = region->vm_end = addr + len;
                         }
                 }

So, how about let proc_reg_get_unmapped_area() return ENOSYS, not EIO?

-- 
Thanks.
HATAYAMA, Daisuke

Re: [PATCH] fix proc_reg_get_unmapped_area()

[Jan Beulich]

>>> On 17.10.13 at 10:36, HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> (2013/10/17 16:23), Jan Beulich wrote:
>>>>> On 16.10.13 at 22:40, Andrew Morton <akpm@linux-foundation.org> wrote:
>>> On Wed, 16 Oct 2013 12:32:40 +0100 "Jan Beulich" <JBeulich@suse.com> wrote:
>>>
>>>> Commit c4fe24485729fc2cbff324c111e67a1cc2f9adea ("sparc: fix PCI device
>>>> proc file mmap(2)"), while fixing one problem, introduced two new ones:
>>>> - the function truncates the return value from ->get_unmapped_area() on
>>>>    64-bit architectures,
>>>> - _all_ descendants are now required to set .get_unmapped_area to non-
>>>>    NULL, which wasn't necessary before (and shouldn't be).
>>>>
>>>> Both - afaict - are a result from a too simplistic copy'n'paste from
>>>> proc_reg_mmap() done in that change.
>>>>
>>>> This likely also addresses reports like the one at
>>>>
>>> 
> http://linux-kernel.2935.n7.nabble.com/mmap-for-proc-vmcore-broken-since-3-12-rc1-td729 
> 
>>> 326.html.
>>>>
>>>> ...
>>>>
>>>> --- 3.12-rc5/fs/proc/inode.c
>>>> +++ 3.12-rc5-proc-get-unmapped-area/fs/proc/inode.c
>>>> @@ -288,12 +288,12 @@ static int proc_reg_mmap(struct file *fi
>>>>   static unsigned long proc_reg_get_unmapped_area(struct file *file, 
> unsigned
>>> long orig_addr, unsigned long len, unsigned long pgoff, unsigned long flags)
>>>>   {
>>>>   	struct proc_dir_entry *pde = PDE(file_inode(file));
>>>> -	int rv = -EIO;
>>>> -	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned
>>> long, unsigned long, unsigned long);
>>>> +	unsigned long rv = -EIO;
>>>> +
>>>>   	if (use_pde(pde)) {
>>>> -		get_unmapped_area = pde->proc_fops->get_unmapped_area;
>>>> -		if (get_unmapped_area)
>>>> -			rv = get_unmapped_area(file, orig_addr, len, pgoff, flags);
>>>> +		rv = (pde->proc_fops->get_unmapped_area
>>>> +		      ?: current->mm->get_unmapped_area)(file, orig_addr, len,
>>>> +							 pgoff, flags);
>>>>   		unuse_pde(pde);
>>>>   	}
>>>>   	return rv;
>>>
>>> I think these two patches will address the problems:
>>>
>>> http://ozlabs.org/~akpm/mmots/broken-out/procfs-fix-unintended-truncation-of-retur 
>>> ned-mapped-address.patch
>>> http://ozlabs.org/~akpm/mmots/broken-out/procfs-call-default-get_unmapped_area-on- 
>>> mmu-present-architectures.patch
>>>
>>> I'll be sending those Linuswards today.  Please check them.  (I think
>>> your version would break the nommu build).
>>
>> Yes indeed - I did search for existing patches, but didn't find them.
>> I see Linus merged them already, so there's no point anymore
>> sending Reviewed-by tags.
>>
>> I think though that in the !MMU case the second of them leaves an
>> issue unfixed nevertheless: If the specific procfs handler has no
>> .get_unmapped_area, the operation would still fail in that case
>> rather than succeed as it did before that wrapper got added.)
>>
>> Jan
>>
> 
> Yes, there remains difference on no-mmu that if mmap is not defined, mmap() 
> now returns
> EIO, it should return ENODEV.

No, afaict it should succeed (and simply use the address that came in).

Jan