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 <ernesto.mnd.fernandez@xxxxxxxxx>
> > ---
> > 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
