Re: [PATCH] hugetlbfs: fix error handling in init_hugetlbfs_fs()

[Chengguang Xu]

 ---- å ææä, 2019-10-29 05:27:01 Mike Kravetz <mike.kravetz@xxxxxxxxxx> æå ----
 > 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@xxxxxxxxxxxx>
 > 
 > 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@xxxxxxxxxx>
 > 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@xxxxxxxxxxxx>
 > Fixes: 32021982a324 ("hugetlbfs: Convert to fs_context")
 > Cc: stable@xxxxxxxxxxxxxxx
 > Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
 > ---
 >  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
 > 
 >