* [PATCH 2/2] hfsplus: add a check for hfs_bnode_find @ 2019-10-16 12:06 Chuhong Yuan 2019-10-17 0:07 ` Ernesto A. Fernández 0 siblings, 1 reply; 5+ messages in thread From: Chuhong Yuan @ 2019-10-16 12:06 UTC (permalink / raw) Cc: linux-fsdevel, linux-kernel, Chuhong Yuan 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@gmail.com> --- 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); /* 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hfsplus: add a check for hfs_bnode_find 2019-10-16 12:06 [PATCH 2/2] hfsplus: add a check for hfs_bnode_find Chuhong Yuan @ 2019-10-17 0:07 ` Ernesto A. Fernández 2019-10-17 1:30 ` Chuhong Yuan 0 siblings, 1 reply; 5+ messages in thread From: Ernesto A. Fernández @ 2019-10-17 0:07 UTC (permalink / raw) To: Chuhong Yuan; +Cc: linux-fsdevel, linux-kernel 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@gmail.com> > --- > 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? > /* 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hfsplus: add a check for hfs_bnode_find 2019-10-17 0:07 ` Ernesto A. Fernández @ 2019-10-17 1:30 ` Chuhong Yuan 2019-10-17 20:52 ` Ernesto A. Fernández 0 siblings, 1 reply; 5+ messages in thread From: Chuhong Yuan @ 2019-10-17 1:30 UTC (permalink / raw) To: Ernesto A. Fernández; +Cc: linux-fsdevel, LKML On Thu, Oct 17, 2019 at 8:07 AM Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> 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@gmail.com> > > --- > > 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. > > /* 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hfsplus: add a check for hfs_bnode_find 2019-10-17 1:30 ` Chuhong Yuan @ 2019-10-17 20:52 ` Ernesto A. Fernández 2019-10-18 8:52 ` Viacheslav Dubeyko 0 siblings, 1 reply; 5+ messages in thread From: Ernesto A. Fernández @ 2019-10-17 20:52 UTC (permalink / raw) To: Chuhong Yuan; +Cc: linux-fsdevel, LKML 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@gmail.com> 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@gmail.com> > > > --- > > > 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. > > > > /* 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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hfsplus: add a check for hfs_bnode_find 2019-10-17 20:52 ` Ernesto A. Fernández @ 2019-10-18 8:52 ` Viacheslav Dubeyko 0 siblings, 0 replies; 5+ messages in thread From: Viacheslav Dubeyko @ 2019-10-18 8:52 UTC (permalink / raw) To: "Ernesto A. Fernández"; +Cc: Chuhong Yuan, linux-fsdevel, LKML 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@gmail.com> 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@gmail.com> 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@gmail.com> >>>> --- >>>> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-18 8:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-16 12:06 [PATCH 2/2] hfsplus: add a check for hfs_bnode_find Chuhong Yuan 2019-10-17 0:07 ` Ernesto A. Fernández 2019-10-17 1:30 ` Chuhong Yuan 2019-10-17 20:52 ` Ernesto A. Fernández 2019-10-18 8:52 ` Viacheslav Dubeyko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).