linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Viacheslav Dubeyko <slava@dubeyko.com>
To: "Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>
Cc: linux-fsdevel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split
Date: Fri, 26 Oct 2018 09:58:49 -0700	[thread overview]
Message-ID: <1540573129.2669.16.camel@dubeyko.com> (raw)
In-Reply-To: <20181025194231.mbfwmt6fhmokrn52@eaf>

On Thu, 2018-10-25 at 16:42 -0300, Ernesto A. Fernández wrote:
> On Thu, Oct 25, 2018 at 09:51:57AM -0700, Viacheslav Dubeyko wrote:
> > 
> > On Tue, 2018-10-23 at 22:32 -0300, Ernesto A. Fernández wrote:
> > > 
> > > On Tue, Oct 23, 2018 at 06:23:53PM -0700, Viacheslav Dubeyko
> > > wrote:
> > > > 
> > > > 
> > > > On Fri, 2018-08-31 at 00:58 -0300, Ernesto A. Fernández wrote:
> > > > > 
> > > > > 
> > > > > Creating, renaming or deleting a file may cause catalog
> > > > > corruption and
> > > > > data loss.  This bug is randomly triggered by xfstests
> > > > > generic/027, but
> > > > > here is a faster reproducer:
> > > > > 
> > > > >   truncate -s 50M fs.iso
> > > > >   mkfs.hfsplus fs.iso
> > > > >   mount fs.iso /mnt
> > > > >   i=100
> > > > >   while [ $i -le 150 ]; do
> > > > >     touch /mnt/$i &>/dev/null
> > > > >     ((++i))
> > > > >   done
> > > > >   i=100
> > > > >   while [ $i -le 150 ]; do
> > > > >     mv /mnt/$i /mnt/$(perl -e "print $i x82") &>/dev/null
> > > > >     ((++i))
> > > > >   done
> > > > >   umount /mnt
> > > > >   fsck.hfsplus -n fs.iso
> > > > > 
> > > > > The bug is triggered whenever hfs_brec_update_parent() needs
> > > > > to
> > > > > split
> > > > > the root node.  The height of the btree is not increased,
> > > > > which
> > > > > leaves
> > > > > the new node orphaned and its records lost.
> > > > > 
> > > > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gm
> > > > > ail.
> > > > > com>
> > > > > ---
> > > > >  fs/hfsplus/brec.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> > > > > index ed8eacb34452..aa17a392b414 100644
> > > > > --- a/fs/hfsplus/brec.c
> > > > > +++ b/fs/hfsplus/brec.c
> > > > > @@ -429,6 +429,10 @@ static int hfs_brec_update_parent(struct
> > > > > hfs_find_data *fd)
> > > > >  	if (new_node) {
> > > > >  		__be32 cnid;
> > > > >  
> > > > > +		if (!new_node->parent) {
> > > > > +			hfs_btree_inc_height(tree);
> > > > > +			new_node->parent = tree->root;
> > > > I worry about the case when we are adding the node on
> > > > intermediate
> > > > (not
> > > > root) level. As far as I can see, we will be in trouble here
> > > > because I
> > > > don't see any processing of two possible cases: (1) root node;
> > > > (2)
> > > > node
> > > > of intermediate level. Do I miss something here?
> > > If 'new_node' had been the result of splitting a node other than
> > > root,
> > > then it would have a parent.
> > > 
> > Could you please share the callstack or/and more detailed
> > explanation
> > that would show the correctness of the fix? Currently, it's not
> > enough
> > details in the comment for easy understanding the issue's
> > environment
> > and correctness of the fix. 
> This patch is two months old now.  Why did you wait until the merge
> window to give me your traditional "the patch is confusing" reply?
> 
> > 
> > I simply mean here that as implementation
> > as concept of the HFS+ b-trees is not trivial. But everybody should
> > easy understand the patch.
> Those two sentences are not compatible.  Reviewers need to have some
> understanding of the code they review.  No royal road...
> 

If you've found an issue or a bug then you did see some error message
and/or callstack. It's not big deal to share it. Moreover, it's
important to include such error message, callstack and issue's details
in the comment section of the patch. Let's imagine that somebody will
use the same test-case and this guy would find another issue. So, how
will it be possible to distinguish your issue with another one for the
same test-case? Does your fix resolve another issue or not? It will be
impossible to realize this without error message or something. I didn't
ask to share some "secret" knowledge. But to share more details about
the issue is really important. Could you please share more details
(error message, callstack and so on) about the found issue? Even if we
are talking about silent corruption then the fsck tool complains about
something. And, finally, you've found some path in the code (callstack)
that is the reason of such corruption. Again, it's not big deal to
share these details.

Thanks,
Vyacheslav Dubeyko. 

  reply	other threads:[~2018-10-27  1:36 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-31  3:58 [PATCH 1/6] hfsplus: prevent btree data loss on root split Ernesto A. Fernández
2018-08-31  3:59 ` [PATCH 2/6] hfsplus: fix BUG on bnode parent update Ernesto A. Fernández
2018-10-24  1:33   ` Viacheslav Dubeyko
2018-10-24  2:48     ` Ernesto A. Fernández
     [not found]       ` <20181024143947.4e30cca3ddda937db70237e9@linux-foundation.org>
2018-10-24 22:45         ` Ernesto A. Fernández
2018-08-31  4:00 ` [PATCH 3/6] hfsplus: prevent btree data loss on ENOSPC Ernesto A. Fernández
2018-08-31  4:00 ` [PATCH 4/6] hfs: prevent btree data loss on root split Ernesto A. Fernández
2018-08-31  4:01 ` [PATCH 5/6] hfs: fix BUG on bnode parent update Ernesto A. Fernández
2018-08-31  4:01 ` [PATCH 6/6] hfs: prevent btree data loss on ENOSPC Ernesto A. Fernández
2018-08-31  5:36 ` [PATCH 1/6] hfsplus: prevent btree data loss on root split Christoph Hellwig
2018-08-31 14:55   ` Ernesto A. Fernández
2018-09-01  4:49     ` Dave Chinner
2018-09-02  4:33       ` Ernesto A. Fernández
2018-09-02 23:32         ` Dave Chinner
2018-09-03  0:06           ` Ernesto A. Fernández
2018-09-06 18:28 ` Ernesto A. Fernández
2018-10-24  1:23 ` Viacheslav Dubeyko
2018-10-24  1:32   ` Ernesto A. Fernández
2018-10-25 16:51     ` Viacheslav Dubeyko
2018-10-25 19:42       ` Ernesto A. Fernández
2018-10-26 16:58         ` Viacheslav Dubeyko [this message]
2018-10-27  5:15           ` Ernesto A. Fernández

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1540573129.2669.16.camel@dubeyko.com \
    --to=slava@dubeyko.com \
    --cc=akpm@linux-foundation.org \
    --cc=ernesto.mnd.fernandez@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).