Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\))
Subject: Re: [PATCH 2/2] hfsplus: add a check for hfs_bnode_find
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <20191017205222.GA2662@eaf>
Date:   Fri, 18 Oct 2019 11:52:44 +0300
Cc:     Chuhong Yuan <hslester96@gmail.com>, linux-fsdevel@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <64BA43A8-4C8A-4284-9263-8549F26659AF@dubeyko.com>
References: <20191016120621.304-1-hslester96@gmail.com>
 <20191017000703.GA4271@eaf>
 <CANhBUQ3vPBAstTMJ25Zt6sR4CcRKWkeR7VKhFXc9aiqQKmW=Ng@mail.gmail.com>
 <20191017205222.GA2662@eaf>
To:     =?utf-8?B?IkVybmVzdG8gQS4gRmVybsOhbmRleiI=?= 
        <ernesto.mnd.fernandez@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Sorry, I had some glitch during message sending. I am repeating the =
message sending.

> On Oct 17, 2019, at 11:52 PM, Ernesto A. Fern=C3=A1ndez =
<ernesto.mnd.fernandez@gmail.com> wrote:
>=20
> On Thu, Oct 17, 2019 at 09:30:20AM +0800, Chuhong Yuan wrote:
>> On Thu, Oct 17, 2019 at 8:07 AM Ernesto A. Fern=C3=A1ndez
>> <ernesto.mnd.fernandez@gmail.com> wrote:
>>>=20
>>> Hi,
>>>=20
>>> On Wed, Oct 16, 2019 at 08:06:20PM +0800, Chuhong Yuan wrote:
>>>> hfs_brec_update_parent misses a check for hfs_bnode_find and may =
miss
>>>> the failure.
>>>> Add a check for it like what is done in again.
>>>>=20
>>>> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
>>>> ---
>>>> fs/hfsplus/brec.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>=20
>>>> diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
>>>> index 1918544a7871..22bada8288c4 100644
>>>> --- a/fs/hfsplus/brec.c
>>>> +++ b/fs/hfsplus/brec.c
>>>> @@ -434,6 +434,8 @@ static int hfs_brec_update_parent(struct =
hfs_find_data *fd)
>>>>                      new_node->parent =3D tree->root;
>>>>              }
>>>>              fd->bnode =3D hfs_bnode_find(tree, new_node->parent);
>>>> +             if (IS_ERR(fd->bnode))
>>>> +                     return PTR_ERR(fd->bnode);
>>>=20
>>> You shouldn't just return here, you still hold a reference to =
new_node.
>>> The call to hfs_bnode_find() after the again label seems to be =
making a
>>> similar mistake.
>>>=20
>>> I don't think either one can actually fail though, because the =
parent
>>> nodes have all been read and hashed before, haven't they?
>>>=20
>>=20
>> I find that after hfs_bnode_findhash in hfs_bnode_find, there is a =
test for
>> HFS_BNODE_ERROR and may return an error. I'm not sure whether it
>> can happen here.
>=20
> That would require a race between hfs_bnode_find() and =
hfs_bnode_create(),
> but the node has already been created.
>=20

The whole function hfs_brec_update_parent() looks like the cycle. And =
there are several
places where PTR_ERR(node) is returned with error ([1] - [2]). So, it =
sounds that it needs
to follow this pattern or to rework these cases too. And, by the way, =
what if the node pointer
will be NULL?

Thanks,
Viacheslav Dubeyko.

[1] =
https://elixir.bootlin.com/linux/latest/source/fs/hfsplus/brec.c#L371
[2] =
https://elixir.bootlin.com/linux/latest/source/fs/hfsplus/brec.c#L402


>>=20
>>>>              /* create index key and entry */
>>>>              hfs_bnode_read_key(new_node, fd->search_key, 14);
>>>>              cnid =3D cpu_to_be32(new_node->this);
>>>> --
>>>> 2.20.1