Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     "Hatayama\, Daisuke" <d.hatayama@jp.fujitsu.com>
Cc:     "'gregkh\@linuxfoundation.org'" <gregkh@linuxfoundation.org>,
        "'tj\@kernel.org'" <tj@kernel.org>,
        "Okajima\, Toshiyuki" <toshi.okajima@jp.fujitsu.com>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "'ebiederm\@aristanetworks.com'" <ebiederm@aristanetworks.com>
References: <33710E6CAA200E4583255F4FB666C4E21B63D491@G01JPEXMBYT03>
        <87wovhqex2.fsf@xmission.com> <87po17r9ek.fsf_-_@xmission.com>
        <33710E6CAA200E4583255F4FB666C4E21B65E04E@G01JPEXMBYT03>
        <87sh62k3vk.fsf@xmission.com>
        <33710E6CAA200E4583255F4FB666C4E21B66BAC8@G01JPEXMBYT03>
Date:   Tue, 05 Jun 2018 10:31:36 -0500
In-Reply-To: <33710E6CAA200E4583255F4FB666C4E21B66BAC8@G01JPEXMBYT03> (Daisuke
        Hatayama's message of "Tue, 5 Jun 2018 05:45:54 +0000")
Message-ID: <87h8mhgsh3.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [CFT][PATCH] kernfs: Correct kernfs directory seeks.
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

"Hatayama, Daisuke" <d.hatayama@jp.fujitsu.com> writes:

>> >> +
>> >> +	/* 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.
>> 
>
> Or when either of the conditions is true, it has resulted in some inconsistent state, right?
> So, why not terminating this session of readdir() immediately by
> returning NULL just as when off is turned out to be invalid?

What I have above is not the clearest, and in fact the logic could be
better.

The fundamental challenge is because hash collisions are possible a file
offset does not hold complete position information in a directory.

So the kernfs node that is to be read/displayed next is saved in the
struct file.  The it is tested if the saved kernfs node is usable
for finding the location in the directory.  Several things may have
gone wrong.

- Someone may have called seekdir.
- The saved kernfs node may have been renamed.
- The saved kernfs node may have been moved to a different directory in
  kernfs.
- the saved kernfs node may have been deleted.

If any of those are true the code needs to do the rbtree lookup.

If the kernfs node has been deleted or moved to a different directory we
can safely use it's name while performing the rbtree lookup.  Which in
the event of a hash collision will be more accurate in finding our old
location, and preventing the same directory entry being returned
multiple times.

Which is completely different than if the directory offset is an invalid
value that will never point to any directory entries.

Eric