> -----Original Message-----
> From: Eric W. Biederman [mailto:ebied...@xmission.com]
> Sent: Tuesday, June 5, 2018 11:03 AM
> To: Hatayama, Daisuke <d.hatay...@jp.fujitsu.com>
> Cc: 'gre...@linuxfoundation.org' <gre...@linuxfoundation.org>;
> 't...@kernel.org' <t...@kernel.org>; Okajima, Toshiyuki
> <toshi.okaj...@jp.fujitsu.com>; linux-kernel@vger.kernel.org;
> 'ebied...@aristanetworks.com' <ebied...@aristanetworks.com>
> Subject: Re: [CFT][PATCH] kernfs: Correct kernfs directory seeks.
>
> ebied...@xmission.com (Eric W. Biederman) writes:
>
> > "Hatayama, Daisuke" <d.hatay...@jp.fujitsu.com> 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 ">".
Though too late, I used fedora 28 and RHEL7.5.
I applied this fix to your patch and the system boot was successfully done.
I'll start testing your patch from now on.
>
> 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
>
