Re: [CFT][PATCH] kernfs: Correct kernfs directory seeks.

From: Eric W. Biederman
Date: Mon Jun 04 2018 - 22:03:16 EST


ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:

> "Hatayama, Daisuke" <d.hatayama@xxxxxxxxxxxxxx> writes:
>
>>> Can you test this and please verify it fixes your issue?
>>
>> I tried this patch on top of v4.17 but the system fails to boot
>> without detecting root disks by dracut like this:
[snip]

>> OTOH, there's no issue on the pure v4.17 kernel.
>>
>> As above, ls /sys/module looks apparently good. But I guess any part of
>> behavior of getdentries() on sysfs must have changed, affecting the disk
>> detection...
>
> I haven't been able to reproduce this yet. My test system boots fine.
> Which fedora are you testing on?

I reproduced something similar and fedora 28. So I think I have found
and fixed the issue. I believe I simply reversed the test at the end of
kernfs_dir_pos. AKA "<" instead of ">".

I am going to see if I can test my changes more throughly on this side
and then repost.



>>> fs/kernfs/dir.c | 109
>>> ++++++++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 67 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
>>> index 89d1dc19340b..8148b5fec48d 100644
>>> --- a/fs/kernfs/dir.c
>>> +++ b/fs/kernfs/dir.c
>>> @@ -1584,53 +1584,75 @@ static int kernfs_dir_fop_release(struct inode *inode,
>>> struct file *filp)
>>> return 0;
>>> }
>>>
>>> +static struct kernfs_node *kernfs_dir_next(struct kernfs_node *pos)
>>> +{
>>> + struct rb_node *node = rb_next(&pos->rb);
>>> + return node ? rb_to_kn(node) : NULL;
>>> +}
>>> +
>>> static struct kernfs_node *kernfs_dir_pos(const void *ns,
>>> - struct kernfs_node *parent, loff_t hash, struct kernfs_node *pos)
>>> + struct kernfs_node *parent, loff_t off, struct kernfs_node *saved)
>>> {
>>> - if (pos) {
>>> - int valid = kernfs_active(pos) &&
>>> - pos->parent == parent && hash == pos->hash;
>>> - kernfs_put(pos);
>>> - if (!valid)
>>> - pos = NULL;
>>> - }
>>> - if (!pos && (hash > 1) && (hash < INT_MAX)) {
>>> - struct rb_node *node = parent->dir.children.rb_node;
>>> - while (node) {
>>> - pos = rb_to_kn(node);
>>> -
>>> - if (hash < pos->hash)
>>> - node = node->rb_left;
>>> - else if (hash > pos->hash)
>>> - node = node->rb_right;
>>> - else
>>> - break;
>>> + struct kernfs_node *pos;
>>> + struct rb_node *node;
>>> + unsigned int hash;
>>> + const char *name = "";
>>> +
>>> + /* Is off a valid name hash? */
>>> + if ((off < 2) || (off >= INT_MAX))
>>> + return NULL;
>>> + hash = off;
>>> +
>>> + /* Is the saved position usable? */
>>> + if (saved) {
>>> + /* Proper parent and hash? */
>>> + if ((parent != saved->parent) || (saved->hash != hash)) {
>>> + saved = NULL;
>>
>> name is uninitialized in this path.
>
> It is. name is initialized to "" see above.
>
>>> + } else {
>>> + if (kernfs_active(saved))
>>> + return saved;
>>> + name = saved->name;
>>> }
>>> }
>>> - /* Skip over entries which are dying/dead or in the wrong namespace
>>> */
>>> - while (pos && (!kernfs_active(pos) || pos->ns != ns)) {
>>> - struct rb_node *node = rb_next(&pos->rb);
>>> - if (!node)
>>> - pos = NULL;
>>> +
>>> + /* Find the closest pos to the hash we are looking for */
>>> + pos = NULL;
>>> + node = parent->dir.children.rb_node;
>>> + while (node) {
>>> + int result;
>>> +
>>> + pos = rb_to_kn(node);
>>> + result = kernfs_name_compare(hash, name, ns, pos);
>>> + if (result < 0)
>>> + node = node->rb_left;
>>> + else if (result > 0)
>>> + node = node->rb_right;
>>> else
>>> - pos = rb_to_kn(node);
>>> + break;
>>> }
>>> +
>>> + /* Ensure pos is at or beyond the target position */
>>> + if (pos && (kernfs_name_compare(hash, name, ns, pos) < 0))
^^^^^^^^^^^^^^^^
should be > 0
>>> + pos = kernfs_dir_next(pos);
>>> +

Eric