In order to avoid using incorrect mnt, we should set
mnt to NULL when we get error from mount_one_hugetlbfs().
Signed-off-by: Chengguang Xu <[email protected]>
---
fs/hugetlbfs/inode.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a478df035651..427d845e7706 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1470,9 +1470,12 @@ static int __init init_hugetlbfs_fs(void)
i = 0;
for_each_hstate(h) {
mnt = mount_one_hugetlbfs(h);
- if (IS_ERR(mnt) && i == 0) {
- error = PTR_ERR(mnt);
- goto out;
+ if (IS_ERR(mnt)) {
+ if (i == 0) {
+ error = PTR_ERR(mnt);
+ goto out;
+ }
+ mnt = NULL;
}
hugetlbfs_vfsmount[i] = mnt;
i++;
--
2.20.1
Sorry for noise, left off David
On 10/17/19 5:08 PM, Mike Kravetz wrote:
> Cc: David
> On 10/17/19 3:38 AM, Chengguang Xu wrote:
>> In order to avoid using incorrect mnt, we should set
>> mnt to NULL when we get error from mount_one_hugetlbfs().
>>
>> Signed-off-by: Chengguang Xu <[email protected]>
>> ---
>> fs/hugetlbfs/inode.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index a478df035651..427d845e7706 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -1470,9 +1470,12 @@ static int __init init_hugetlbfs_fs(void)
>> i = 0;
>> for_each_hstate(h) {
>> mnt = mount_one_hugetlbfs(h);
>> - if (IS_ERR(mnt) && i == 0) {
>> - error = PTR_ERR(mnt);
>> - goto out;
>> + if (IS_ERR(mnt)) {
>> + if (i == 0) {
>> + error = PTR_ERR(mnt);
>> + goto out;
>> + }
>> + mnt = NULL;
>> }
>> hugetlbfs_vfsmount[i] = mnt;
>> i++;
>
> Thanks!
>
> That should be fixed. It was introduced with commit 32021982a324 ("hugetlbfs:
> Convert to fs_context").
>
> That commit also changed the condition for which init_hugetlbfs_fs() would
> 'error' and remove the inode cache. Previously, it would do that if there
> was an error creating a mount for the default_hstate_idx hstate. It now does
> that for the '0' hstate, and 0 is not always equal to default_hstate_idx.
>
> David was that intentional or an oversight? I can fix up, just wanted to
> make sure there was not some reason for the change.
>
--
Mike Kravetz
Cc: David
On 10/17/19 3:38 AM, Chengguang Xu wrote:
> In order to avoid using incorrect mnt, we should set
> mnt to NULL when we get error from mount_one_hugetlbfs().
>
> Signed-off-by: Chengguang Xu <[email protected]>
> ---
> fs/hugetlbfs/inode.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index a478df035651..427d845e7706 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1470,9 +1470,12 @@ static int __init init_hugetlbfs_fs(void)
> i = 0;
> for_each_hstate(h) {
> mnt = mount_one_hugetlbfs(h);
> - if (IS_ERR(mnt) && i == 0) {
> - error = PTR_ERR(mnt);
> - goto out;
> + if (IS_ERR(mnt)) {
> + if (i == 0) {
> + error = PTR_ERR(mnt);
> + goto out;
> + }
> + mnt = NULL;
> }
> hugetlbfs_vfsmount[i] = mnt;
> i++;
Thanks!
That should be fixed. It was introduced with commit 32021982a324 ("hugetlbfs:
Convert to fs_context").
That commit also changed the condition for which init_hugetlbfs_fs() would
'error' and remove the inode cache. Previously, it would do that if there
was an error creating a mount for the default_hstate_idx hstate. It now does
that for the '0' hstate, and 0 is not always equal to default_hstate_idx.
David was that intentional or an oversight? I can fix up, just wanted to
make sure there was not some reason for the change.
--
Mike Kravetz
On 10/17/19 3:38 AM, Chengguang Xu wrote:
> In order to avoid using incorrect mnt, we should set
> mnt to NULL when we get error from mount_one_hugetlbfs().
>
> Signed-off-by: Chengguang Xu <[email protected]>
Thanks for noticing this issue. As mentioned in a previous e-mail,
there are additional issues that need to be addressed. This loop
needs to initialize entries in the hugetlbfs_vfsmount array for all
hstates. How about this patch?
From 3144f0a9d18f1408e831fb3eb49a06636a11d902 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Mon, 28 Oct 2019 14:08:42 -0700
Subject: [PATCH] mm/hugetlbfs: fix error handling when setting up mounts
It is assumed that the hugetlbfs_vfsmount[] array will contain
either a valid vfsmount pointer or NULL for each hstate after
initialization. Changes made while converting to use fs_context
broke this assumption.
Reported-by: Chengguang Xu <[email protected]>
Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context")
Cc: [email protected]
Signed-off-by: Mike Kravetz <[email protected]>
---
fs/hugetlbfs/inode.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a478df035651..178389209561 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1470,15 +1470,17 @@ static int __init init_hugetlbfs_fs(void)
i = 0;
for_each_hstate(h) {
mnt = mount_one_hugetlbfs(h);
- if (IS_ERR(mnt) && i == 0) {
+ if (IS_ERR(mnt)) {
+ hugetlbfs_vfsmount[i] = NULL;
error = PTR_ERR(mnt);
- goto out;
+ } else {
+ hugetlbfs_vfsmount[i] = mnt;
}
- hugetlbfs_vfsmount[i] = mnt;
i++;
}
- return 0;
+ if (hugetlbfs_vfsmount[default_hstate_idx] != NULL)
+ return 0;
out:
kmem_cache_destroy(hugetlbfs_inode_cachep);
--
2.20.1
---- 在 星期二, 2019-10-29 05:27:01 Mike Kravetz <[email protected]> 撰写 ----
> On 10/17/19 3:38 AM, Chengguang Xu wrote:
> > In order to avoid using incorrect mnt, we should set
> > mnt to NULL when we get error from mount_one_hugetlbfs().
> >
> > Signed-off-by: Chengguang Xu <[email protected]>
>
> Thanks for noticing this issue. As mentioned in a previous e-mail,
> there are additional issues that need to be addressed. This loop
> needs to initialize entries in the hugetlbfs_vfsmount array for all
> hstates. How about this patch?
>
> From 3144f0a9d18f1408e831fb3eb49a06636a11d902 Mon Sep 17 00:00:00 2001
> From: Mike Kravetz <[email protected]>
> Date: Mon, 28 Oct 2019 14:08:42 -0700
> Subject: [PATCH] mm/hugetlbfs: fix error handling when setting up mounts
>
> It is assumed that the hugetlbfs_vfsmount[] array will contain
> either a valid vfsmount pointer or NULL for each hstate after
> initialization. Changes made while converting to use fs_context
> broke this assumption.
>
> Reported-by: Chengguang Xu <[email protected]>
> Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context")
> Cc: [email protected]
> Signed-off-by: Mike Kravetz <[email protected]>
> ---
> fs/hugetlbfs/inode.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index a478df035651..178389209561 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1470,15 +1470,17 @@ static int __init init_hugetlbfs_fs(void)
> i = 0;
> for_each_hstate(h) {
> mnt = mount_one_hugetlbfs(h);
> - if (IS_ERR(mnt) && i == 0) {
> + if (IS_ERR(mnt)) {
> + hugetlbfs_vfsmount[i] = NULL;
> error = PTR_ERR(mnt);
> - goto out;
> + } else {
> + hugetlbfs_vfsmount[i] = mnt;
> }
> - hugetlbfs_vfsmount[i] = mnt;
> i++;
> }
>
> - return 0;
> + if (hugetlbfs_vfsmount[default_hstate_idx] != NULL)
> + return 0;
Maybe we should umount other non-null entries and release
used inodes for safety in error case.
Thanks,
Chengguang
>
> out:
> kmem_cache_destroy(hugetlbfs_inode_cachep);
> --
> 2.20.1
>
>
On 10/28/19 9:36 PM, Chengguang Xu wrote:
> ---- 在 星期二, 2019-10-29 05:27:01 Mike Kravetz <[email protected]> 撰写 ----
> > Subject: [PATCH] mm/hugetlbfs: fix error handling when setting up mounts
> >
> > It is assumed that the hugetlbfs_vfsmount[] array will contain
> > either a valid vfsmount pointer or NULL for each hstate after
> > initialization. Changes made while converting to use fs_context
> > broke this assumption.
> >
> > Reported-by: Chengguang Xu <[email protected]>
> > Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context")
> > Cc: [email protected]
> > Signed-off-by: Mike Kravetz <[email protected]>
> > ---
> > fs/hugetlbfs/inode.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index a478df035651..178389209561 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -1470,15 +1470,17 @@ static int __init init_hugetlbfs_fs(void)
> > i = 0;
> > for_each_hstate(h) {
> > mnt = mount_one_hugetlbfs(h);
> > - if (IS_ERR(mnt) && i == 0) {
> > + if (IS_ERR(mnt)) {
> > + hugetlbfs_vfsmount[i] = NULL;
> > error = PTR_ERR(mnt);
> > - goto out;
> > + } else {
> > + hugetlbfs_vfsmount[i] = mnt;
> > }
> > - hugetlbfs_vfsmount[i] = mnt;
> > i++;
> > }
> >
> > - return 0;
> > + if (hugetlbfs_vfsmount[default_hstate_idx] != NULL)
> > + return 0;
>
> Maybe we should umount other non-null entries and release
> used inodes for safety in error case.
Yes, even the original code did not clean up properly in the case of
all mount errors. This will explicitly mount the default_hstate_idx
hstate first. Then, optionally mount other hstates. It will make the
error handling and cleanup more explicit.
From 7291f1da0d494cb64f6d219943c59a02e6d4fca7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Mon, 28 Oct 2019 14:08:42 -0700
Subject: [PATCH] mm/hugetlbfs: fix error handling when setting up mounts
It is assumed that the hugetlbfs_vfsmount[] array will contain
either a valid vfsmount pointer or NULL for each hstate after
initialization. Changes made while converting to use fs_context
broke this assumption.
While fixing the hugetlbfs_vfsmount issue, it was discovered that
init_hugetlbfs_fs never did correctly clean up when encountering
a vfs mount error.
Reported-by: Chengguang Xu <[email protected]>
Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context")
Cc: [email protected]
Signed-off-by: Mike Kravetz <[email protected]>
---
fs/hugetlbfs/inode.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a478df035651..26e3906c18fe 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1461,28 +1461,41 @@ static int __init init_hugetlbfs_fs(void)
sizeof(struct hugetlbfs_inode_info),
0, SLAB_ACCOUNT, init_once);
if (hugetlbfs_inode_cachep == NULL)
- goto out2;
+ goto out;
error = register_filesystem(&hugetlbfs_fs_type);
if (error)
- goto out;
+ goto out_free;
+ /* default hstate mount is required */
+ mnt = mount_one_hugetlbfs(&hstates[default_hstate_idx]);
+ if (IS_ERR(mnt)) {
+ error = PTR_ERR(mnt);
+ goto out_unreg;
+ }
+ hugetlbfs_vfsmount[default_hstate_idx] = mnt;
+
+ /* other hstates are optional */
i = 0;
for_each_hstate(h) {
+ if (i == default_hstate_idx)
+ continue;
+
mnt = mount_one_hugetlbfs(h);
- if (IS_ERR(mnt) && i == 0) {
- error = PTR_ERR(mnt);
- goto out;
- }
- hugetlbfs_vfsmount[i] = mnt;
+ if (IS_ERR(mnt))
+ hugetlbfs_vfsmount[i] = NULL;
+ else
+ hugetlbfs_vfsmount[i] = mnt;
i++;
}
return 0;
- out:
+ out_unreg:
+ (void)unregister_filesystem(&hugetlbfs_fs_type);
+ out_free:
kmem_cache_destroy(hugetlbfs_inode_cachep);
- out2:
+ out:
return error;
}
fs_initcall(init_hugetlbfs_fs)
--
2.20.1
On Tue, 29 Oct 2019 13:47:38 -0700 Mike Kravetz <[email protected]> wrote:
> It is assumed that the hugetlbfs_vfsmount[] array will contain
> either a valid vfsmount pointer or NULL for each hstate after
> initialization. Changes made while converting to use fs_context
> broke this assumption.
>
> While fixing the hugetlbfs_vfsmount issue, it was discovered that
> init_hugetlbfs_fs never did correctly clean up when encountering
> a vfs mount error.
What were the user-visible runtime effects of this bug?
(IOW: why does it warrant the cc:stable?)
On 10/29/19 3:24 PM, Andrew Morton wrote:
> On Tue, 29 Oct 2019 13:47:38 -0700 Mike Kravetz <[email protected]> wrote:
>
>> It is assumed that the hugetlbfs_vfsmount[] array will contain
>> either a valid vfsmount pointer or NULL for each hstate after
>> initialization. Changes made while converting to use fs_context
>> broke this assumption.
>>
>> While fixing the hugetlbfs_vfsmount issue, it was discovered that
>> init_hugetlbfs_fs never did correctly clean up when encountering
>> a vfs mount error.
>
> What were the user-visible runtime effects of this bug?
>
> (IOW: why does it warrant the cc:stable?)
On second thought, let's not cc stable.
It was found during code inspection. A small memory allocation failure
would be the most likely cause of taking a error path with the bug. This
is unlikely to happen as this is early init code.
Sorry about that,
--
Mike Kravetz