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

[Hatayama, Daisuke]

> -----Original Message-----
> From: Eric W. Biederman [mailto:ebiederm@xxxxxxxxxxxx]
> Sent: Tuesday, June 5, 2018 11:03 AM
> To: Hatayama, Daisuke <d.hatayama@xxxxxxxxxxxxxx>
> Cc: 'gregkh@xxxxxxxxxxxxxxxxxxx' <gregkh@xxxxxxxxxxxxxxxxxxx>;
> 'tj@xxxxxxxxxx' <tj@xxxxxxxxxx>; Okajima, Toshiyuki 
> <toshi.okajima@xxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> 'ebiederm@xxxxxxxxxxxxxxxxxx' <ebiederm@xxxxxxxxxxxxxxxxxx>
> Subject: Re: [CFT][PATCH] kernfs: Correct kernfs directory seeks.
> 
> 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 ">".

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
>