From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f179.google.com ([209.85.220.179]:55147 "EHLO mail-qk0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753753AbdJHTrA (ORCPT ); Sun, 8 Oct 2017 15:47:00 -0400 Received: by mail-qk0-f179.google.com with SMTP id n5so19721987qke.11 for ; Sun, 08 Oct 2017 12:46:59 -0700 (PDT) Date: Sun, 8 Oct 2017 16:46:54 -0300 From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= To: Viacheslav Dubeyko , linux-fsdevel@vger.kernel.org Cc: Sergei Antonov , Hin-Tak Leung , Al Viro , Christoph Hellwig , Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= Subject: Re: [PATCH] hfsplus: fix segfault when deleting all attrs of a file Message-ID: <20171008194653.GA2196@debian.home> References: <20171006215222.GA4736@debian.home> <1507352581.2512.30.camel@dubeyko.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1507352581.2512.30.camel@dubeyko.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, Oct 06, 2017 at 10:03:01PM -0700, Viacheslav Dubeyko wrote: > On Fri, 2017-10-06 at 18:52 -0300, Ernesto A. Fernández wrote: > > A segmentation fault can be triggered by setting many xattrs to a > > file > > and then deleting it. The number must be high enough for more than > > one > > b-tree node to be needed for storage. > > > > I think it could be great to see in the description: > (1) The explanation of the reproducing path of the issue; > (2) The callstack of the crash and more details about it (if you are > able to reproduce the issue, of course). > > Could you please add these details in the description? I sent the script I used in a separate mail, would that be enough help? It seems from your review that you already understood all that the callstack can tell you. Maybe I should add the script to the commit message? > > > When hfs_brec_remove() is called as part of > > hfsplus_delete_all_attrs(), > > fd->search_key will not be set to any specific value. It does not > > matter > > because we intend to remove all records for a given cnid. > > > I am not fully follow to the issue. The hfsplus_delete_all_attrs calls > hfsplus_find_attr() before hfsplus_delete_attr(): > > for (;;) { > err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd); > if (err) { > if (err != -ENOENT) > pr_err("xattr search failed\n"); > goto end_delete_all; > } > > err = __hfsplus_delete_attr(dir, cnid, &fd); > if (err) > goto end_delete_all; > } > > The hfsplus_find_attr() prepares fd->search_key: > > if (name) { > err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name); > if (err) > goto failed_find_attr; > err = hfs_brec_find(fd, hfs_find_rec_by_key); > if (err) > goto failed_find_attr; > } else { > err = hfsplus_attr_build_key(sb, fd->search_key, cnid, NULL); Here hfsplus_attr_build_key will only prepare the key for a search by cnid. We do not yet know the name of the xattr (that's what the NULL is) so it can't be set to anything specific. > if (err) > goto failed_find_attr; > err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid); > if (err) > goto failed_find_attr; > } > > And, finally, __hfsplus_delete_attr() calls the hfs_brec_remove(). I am > not follow to the explanation of the issue. Maybe I missed something? The problem is that hfs_brec_remove() assumes that fd was prepared for a search by key, but it was prepared for a search by cnid. It will fail to find the parent records. > Could you share the callstack of the crash andd more details? > > > > > The problem is that hfs_brec_remove() assumes it is being called with > > the result of a search by key, not by cnid. The value of search_key > > may > > be used to update the parent nodes. When no appropriate parent record > > is > > found, the result is an out of bounds access. > > > > To fix this, set the value of fd->search_key to the key of the first > > record in the node, which is also the key of the corresponding parent > > record. > > > > Signed-off-by: Ernesto A. Fernández > > --- > >  fs/hfsplus/brec.c | 3 +++ > >  1 file changed, 3 insertions(+) > > > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > > index 754fdf8..dfa60cf 100644 > > --- a/fs/hfsplus/brec.c > > +++ b/fs/hfsplus/brec.c > > @@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd) > >   > >   tree = fd->tree; > >   node = fd->bnode; > > + > > + /* in case we need to search the parent node */ > > + hfs_bnode_read_key(node, fd->search_key, 14); > > The hardcoded value looks really weird. Could you rework this by means > of adding some constant with reasonable name? Why 14? I agree that it's weird, but the number 14 is all over brec.c. It would be more confusing if I used a constant only this one time, so I decided to respect the style that was in use. To be clear, 14 is sizeof(hfs_bnode_desc), that is, the size of the header of the bnode, and the position of the first record. If you find it better, maybe we can change 14 to sizeof(hfs_bnode_desc) every time it appears in the file, but I think it should be a separate patch. > > >  again: > >   rec_off = tree->node_size - (fd->record + 2) * 2; > >   end_off = tree->node_size - (node->num_recs + 1) * 2; > > Thanks, > Vyacheslav Dubeyko. > > Thank you for your review, Ernest