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?
> 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);
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?
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?
> again:
> rec_off = tree->node_size - (fd->record + 2) * 2;
> end_off = tree->node_size - (node->num_recs + 1) * 2;
Thanks,
Vyacheslav Dubeyko.
