On Sun, 2017-10-08 at 16:46 -0300, Ernesto A. Fernández wrote:
> 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?
>
The script is the way to reproduce the issue. But the callstack of the
crash will be the explanation of the symptoms and the environment of
the issue. Also the callstack provides the opportunity to analyze the
issue.
> >
> >
> > >
> > > 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.
>
I still cannot follow what the real cause of the issue. This is why I
requested the callstack of the crash. Could you point out the code line
in hfs_brec_remove() that is the place of the crash? I don't see the
place where hfs_brec_remove() is trying to access the fd->search_key
directly. It sounds for me that the real trouble is not in the
hfs_brec_remove().
Let's imagine that we have trouble with fd->search_key in the
hfs_brec_remove(). But then it means that the real fix should be
in hfs_brec_find() or hfs_find_1st_rec_by_cnid(). But, currently, I am
not convinced by your explanation that the reason is there where you
did fix it.
The struct hfsplus_attr_key is simply the piece of memory:
struct hfsplus_attr_key {
__be16 key_len;
__be16 pad;
hfsplus_cnid cnid;
__be32 start_block;
struct hfsplus_attr_unistr key_name;
} __packed;
And the hfsplus_attr_build_key() initializes this piece of memory:
int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
u32 cnid, const char *name)
{
int len;
memset(key, 0, sizeof(struct hfsplus_attr_key));
key->attr.cnid = cpu_to_be32(cnid);
if (name) {
int res = hfsplus_asc2uni(sb,
(struct hfsplus_unistr *)&key->attr.key_name,
HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
if (res)
return res;
len = be16_to_cpu(key->attr.key_name.length);
} else {
key->attr.key_name.length = 0;
len = 0;
}
/* The length of the key, as stored in key_len field, does not include
* the size of the key_len field itself.
* So, offsetof(hfsplus_attr_key, key_name) is a trick because
* it takes into consideration key_len field (__be16) of
* hfsplus_attr_key structure instead of length field (__be16) of
* hfsplus_attr_unistr structure.
*/
key->key_len =
cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
2 * len);
return 0;
}
So, even if we haven't name in this structure then the attr.key_name
will have zero length and the buffer will contain the string is filled
by zeros. And it's not clear for me how this can be the reason of
crash? Could you share the callstack of the crash, finally? I still
don't follow your vision. Please, explain the issue and your fix in
more clear and detailed way.
It is possible to do the search by name only or by cnid only.
Everything should work OK. And I didn't rememer any troubles
with hfsplus_delete_all_attrs() on the implementation and testing
phase. I think you could have some specific environment or maybe the
code base was changed dramatically. But, anyway, I don't follow your
vision of the issue.
> >
> > 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@gmail.
> > > com>
> > > ---
> > > 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.
>
The using of sizeof(hfs_bnode_desc) will be pretty good and clean
solution.
Thanks,
Vyacheslav Dubeyko.
