linux-kernel - Re: [PATCH] xfs: check the return value of krealloc() in xfs_uuid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201126021622.GA336866@xiangao.remote.csb>
Date:   Thu, 26 Nov 2020 10:16:22 +0800
From:   Gao Xiang <hsiangkao@...hat.com>
To:     Qinglang Miao <miaoqinglang@...wei.com>
Cc:     Eric Sandeen <sandeen@...deen.net>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: check the return value of krealloc() in
 xfs_uuid_mount

Hi Qinglang,

On Thu, Nov 26, 2020 at 09:21:11AM +0800, Qinglang Miao wrote:
> 
> 
> 在 2020/11/25 23:55, Eric Sandeen 写道:
> > On 11/25/20 12:50 AM, Qinglang Miao wrote:
> > > krealloc() may fail to expand the memory space.
> > 
> > Even with __GFP_NOFAIL?
> > 
> >    * ``GFP_KERNEL | __GFP_NOFAIL`` - overrides the default allocator behavior
> >      and all allocation requests will loop endlessly until they succeed.
> >      This might be really dangerous especially for larger orders.
> > 
> > > Add sanity checks to it,
> > > and WARN() if that really happened.
> > 
> > As aside, there is no WARN added in this patch for a memory failure.
> > 
> > > Fixes: 771915c4f688 ("xfs: remove kmem_realloc()")
> > > Reported-by: Hulk Robot <hulkci@...wei.com>
> > > Signed-off-by: Qinglang Miao <miaoqinglang@...wei.com>
> > > ---
> > >   fs/xfs/xfs_mount.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index 150ee5cb8..c07f48c32 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
> > > @@ -80,9 +80,13 @@ xfs_uuid_mount(
> > >   	}
> > >   	if (hole < 0) {
> > > -		xfs_uuid_table = krealloc(xfs_uuid_table,
> > > +		uuid_t *if_xfs_uuid_table;
> > > +		if_xfs_uuid_table = krealloc(xfs_uuid_table,
> > >   			(xfs_uuid_table_size + 1) * sizeof(*xfs_uuid_table),
> > >   			GFP_KERNEL | __GFP_NOFAIL);
> > > +		if (!if_xfs_uuid_table)
> > > +			goto out_duplicate;
> > 
> > And this would emit "Filesystem has duplicate UUID" which is not correct.
> > 
> > But anyway, the __GFP_NOFAIL in the call makes this all moot AFAICT.
> > 
> > -Eric
> Hi Eric,
> 
> Sorry for neglecting __GFP_NOFAIL symbol, and I would add a WARN in memory
> failure next time.

Sorry about my limited knowledge, but why it needs a WARN here since
I think it will never fail if __GFP_NOFAIL is added (no ?).

I'm not sure if Hulk CI is completely broken or not on this, also if
such CI can now generate trivial patch (?) since the subject, commit
message and even the variable name is quite similiar to
https://lore.kernel.org/linux-xfs/20201124104531.561-2-thunder.leizhen@huawei.com
in a day.

And it'd be better to look into the code before sending patches...

Thanks,
Gao Xiang

> 
> Thanks for your advice！
>