Message ID | 20191017103822.8610-1-cgxu519@mykernel.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hugetlbfs: fix error handling in init_hugetlbfs_fs() | expand |
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 <cgxu519@mykernel.net> > --- > 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.
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 <cgxu519@mykernel.net> >> --- >> 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. >
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 <cgxu519@mykernel.net> 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 <mike.kravetz@oracle.com> 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 <cgxu519@mykernel.net> Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context") Cc: stable@vger.kernel.org Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- 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);
---- 在 星期二, 2019-10-29 05:27:01 Mike Kravetz <mike.kravetz@oracle.com> 撰写 ---- > 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 <cgxu519@mykernel.net> > > 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 <mike.kravetz@oracle.com> > 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 <cgxu519@mykernel.net> > Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context") > Cc: stable@vger.kernel.org > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > 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 <mike.kravetz@oracle.com> 撰写 ---- > > 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 <cgxu519@mykernel.net> > > Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context") > > Cc: stable@vger.kernel.org > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > > --- > > 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 <mike.kravetz@oracle.com> 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 <cgxu519@mykernel.net> Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context") Cc: stable@vger.kernel.org Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- 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)
On Tue, 29 Oct 2019 13:47:38 -0700 Mike Kravetz <mike.kravetz@oracle.com> 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 <mike.kravetz@oracle.com> 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,
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++;
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 <cgxu519@mykernel.net> --- fs/hugetlbfs/inode.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)