[PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Dafna Hirschfeld]

The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
further translated in EFAULT in i.e. pin/get_user_pages.
But when running out of pages/hugepages we expect to see ENOMEM and
not EFAULT.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@intel.com>
---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index cec4b121193f..5c8de0f5c760 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3113,7 +3113,7 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
 	if (!memcg_charge_ret)
 		mem_cgroup_cancel_charge(memcg, nr_pages);
 	mem_cgroup_put(memcg);
-	return ERR_PTR(-ENOSPC);
+	return ERR_PTR(-ENOMEM);
 }
 
 int alloc_bootmem_huge_page(struct hstate *h, int nid)
-- 
2.34.1

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Andrew Morton]

On Sun,  1 Dec 2024 03:03:41 +0200 Dafna Hirschfeld <dafna.hirschfeld@intel.com> wrote:

> The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
> further translated in EFAULT in i.e. pin/get_user_pages.
> But when running out of pages/hugepages we expect to see ENOMEM and
> not EFAULT.
> 
> ...
>
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3113,7 +3113,7 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
>  	if (!memcg_charge_ret)
>  		mem_cgroup_cancel_charge(memcg, nr_pages);
>  	mem_cgroup_put(memcg);
> -	return ERR_PTR(-ENOSPC);
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  int alloc_bootmem_huge_page(struct hstate *h, int nid)

err, yes.  ENOSPC is for disk drives!  I'll slap a cc:stable on this
fix for a decade old bug.

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Matthew Wilcox]

On Sun, Dec 01, 2024 at 03:03:41AM +0200, Dafna Hirschfeld wrote:
> The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
> further translated in EFAULT in i.e. pin/get_user_pages.
> But when running out of pages/hugepages we expect to see ENOMEM and
> not EFAULT.

Do we?  ENOSPC is for media full, and since memory is the medium, ENOSPC
seems entirely appropriate to me.  It's been this way since 2008, so
any change should be done with due care and attention to what else we
might break.

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Muchun Song]

> On Dec 1, 2024, at 09:03, Dafna Hirschfeld <dafna.hirschfeld@intel.com> wrote:
> 
> The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
> further translated in EFAULT in i.e. pin/get_user_pages.
> But when running out of pages/hugepages we expect to see ENOMEM and
> not EFAULT.

Hi Dafna,

Refers to Documentation/mm/hugetlbfs_reserv.rst. I saw:

    If no huge page exists at page fault time, the task is sent
    a **SIGBUS** and often dies an unhappy death.

Seems SIGBUS is expected since it is introduced.

Thanks.

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Dafna Hirschfeld]

On 02.12.2024 11:49, Muchun Song wrote:
>
>
>> On Dec 1, 2024, at 09:03, Dafna Hirschfeld <dafna.hirschfeld@intel.com> wrote:
>>
>> The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
>> further translated in EFAULT in i.e. pin/get_user_pages.
>> But when running out of pages/hugepages we expect to see ENOMEM and
>> not EFAULT.
>
>Hi Dafna,
>
>Refers to Documentation/mm/hugetlbfs_reserv.rst. I saw:
>
>    If no huge page exists at page fault time, the task is sent
>    a **SIGBUS** and often dies an unhappy death.
>
>Seems SIGBUS is expected since it is introduced.

Ok, but for functions such as pin/get_user_pages, it is translated to EFAULT,
which is not clear if this is what we want.


>
>Thanks.

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Dafna Hirschfeld]

On 02.12.2024 11:49, Muchun Song wrote:
>
>
>> On Dec 1, 2024, at 09:03, Dafna Hirschfeld <dafna.hirschfeld@intel.com> wrote:
>>
>> The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
>> further translated in EFAULT in i.e. pin/get_user_pages.
>> But when running out of pages/hugepages we expect to see ENOMEM and
>> not EFAULT.
>
>Hi Dafna,
>
>Refers to Documentation/mm/hugetlbfs_reserv.rst. I saw:
>
>    If no huge page exists at page fault time, the task is sent
>    a **SIGBUS** and often dies an unhappy death.
>
>Seems SIGBUS is expected since it is introduced.

Hi, thanks for pointing this out, the question is wheather EFAULT
is the expected return code from pin/get_user_pages* funcs in such case.
Dafna
>
>Thanks.

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Dafna Hirschfeld]

On 02.12.2024 11:49, Muchun Song wrote:
>
>
>> On Dec 1, 2024, at 09:03, Dafna Hirschfeld <dafna.hirschfeld@intel.com> wrote:
>>
>> The error ENOSPC is translated in vmf_error to VM_FAULT_SIGBUS which is
>> further translated in EFAULT in i.e. pin/get_user_pages.
>> But when running out of pages/hugepages we expect to see ENOMEM and
>> not EFAULT.
>
>Hi Dafna,
>
>Refers to Documentation/mm/hugetlbfs_reserv.rst. I saw:
>
>    If no huge page exists at page fault time, the task is sent
>    a **SIGBUS** and often dies an unhappy death.
>
>Seems SIGBUS is expected since it is introduced.

Hi,
I see that mlock do return ENOMEM when out of hugepages.
It has the function "__mlock_posix_error_return" that converts EFAULT returned from
"get_user_pages" to ENOMEM.
So callers to "get_user_pages" can override EFAULT with ENOMEM but then it obscure
cases where EFUALT should really be the return code.

thanks,
Dafna

>
>Thanks.

Re: [PATCH] mm/hugetlb: change ENOSPC to ENOMEM in alloc_hugetlb_folio

[Alexander Gordeev]

On Sat, Nov 30, 2024 at 06:51:21PM -0800, Andrew Morton wrote:

Hi Andrew,
...
> > -	return ERR_PTR(-ENOSPC);
> > +	return ERR_PTR(-ENOMEM);
...
> err, yes.  ENOSPC is for disk drives!  I'll slap a cc:stable on this
> fix for a decade old bug.

This fix invalidates the comment in hugetlb_no_page():

	/*
	 * Returning error will result in faulting task being
	 * sent SIGBUS.  The hugetlb fault mutex prevents two
	   ...
	 */

The problem is an attempt to write to a MAP_NORESERVE huge page (as per [1])
causes an infinite page fault cycle instead of SIGBUS, at least on s390:

	q = mmap(NULL, hpage_size,
		PROT_READ | PROT_WRITE, MAP_SHARED | MAP_NORESERVE, fd2, 0);
	...
	test_write(q);

1. https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/noresv-preserve-resv-page.c

Thanks!