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ández <ernesto.mnd.fernandez@xxxxxxxxx> wrote: > > 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ández >> <ernesto.mnd.fernandez@xxxxxxxxx> wrote: >>> >>> Hi, >>> >>> 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. >>>> >>>> Signed-off-by: Chuhong Yuan <hslester96@xxxxxxxxx> >>>> --- >>>> fs/hfsplus/brec.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> 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 = tree->root; >>>> } >>>> fd->bnode = hfs_bnode_find(tree, new_node->parent); >>>> + if (IS_ERR(fd->bnode)) >>>> + return PTR_ERR(fd->bnode); >>> >>> 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. >>> >>> I don't think either one can actually fail though, because the parent >>> nodes have all been read and hashed before, haven't they? >>> >> >> 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. > > That would require a race between hfs_bnode_find() and hfs_bnode_create(), > but the node has already been created. > 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 >> >>>> /* create index key and entry */ >>>> hfs_bnode_read_key(new_node, fd->search_key, 14); >>>> cnid = cpu_to_be32(new_node->this); >>>> -- >>>> 2.20.1
