linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Linux-cifs readdir behaviour when dir modified
@ 2020-10-20  5:44 Shyam Prasad N
  2020-10-21  2:21 ` Matthew Wilcox
  0 siblings, 1 reply; 3+ messages in thread
From: Shyam Prasad N @ 2020-10-20  5:44 UTC (permalink / raw)
  To: CIFS, linux-fsdevel, Steve French

Hi,

I spent some time in debugging this issue today:
https://gitlab.alpinelinux.org/alpine/aports/-/issues/10960

A summary of the issue:
With alpine linux containers (which uses the musl implementation of
libc), the "rm -Rf" command could fail depending upon the dir size.
The Linux cifs client filesystem behaviour is compared against ext4
behaviour.

What I saw while debugging (some of which is already covered in the
bug and related bugs):
1. The musl libc sends down small buffers as a part of it's readdir
implementation. These buffers are very small compared to it's glibc
counterpart.
2. cifs.ko is reading the whole directory from the server, but is only
able to fill the readdir results in small portions due to the bufsize
sent by the libc.
3. The libc does unlink of the dirents that have been returned so far.
4. The libc continues the readdir from the prev offset, expecting to
continue the listing.
5. cifs.ko now sees that the directory has changed, and fetches the
directory contents from the server again. However, the reply to the
user is populated from the prev offset, so directory listing does not
return files from the new beginning.
6. As a result, the final rmdir (which assumes that the directory is
now empty) fails.

Out of curiosity, I checked the ext4 code to understand the handling
of this use case, and I see this comment:
        /* If the dir block has changed since the last call to
                                        * readdir(2), then we might be
pointing to an invalid                                               *
dirent right now.  Scan from the start of the block
                             * to make sure. */
... and the corresponding code.

Now the question is whether cifs.ko is doing anything wrong?
@Steve French pointed me to this readdir documentation:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir_r.html

If a file is removed from or added to the directory after the most
recent call to opendir() or rewinddir(), whether a subsequent call to
readdir() returns an entry for that file is unspecified.

So I guess the documents don't specify the behaviour in this case.

We could go the ext4 way and reset the offset to 0 when we detect that
the directory has been modified. That would handle the "rm -Rf" use
case as expected by the user here. However, we could end up repeating
dirents over successive readdir calls.

Posting the question to the larger group, to see if it's worth the
effort to make this change. The change here seems quite simple, which
is to reset file->pos to 0, when we detect that the dir has changed.
But since it'll result in change in behaviour, I wanted to check
first.

-- 
-Shyam

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Linux-cifs readdir behaviour when dir modified
  2020-10-20  5:44 Linux-cifs readdir behaviour when dir modified Shyam Prasad N
@ 2020-10-21  2:21 ` Matthew Wilcox
  2020-10-21  3:25   ` Shyam Prasad N
  0 siblings, 1 reply; 3+ messages in thread
From: Matthew Wilcox @ 2020-10-21  2:21 UTC (permalink / raw)
  To: Shyam Prasad N; +Cc: CIFS, linux-fsdevel, Steve French

On Tue, Oct 20, 2020 at 11:14:11AM +0530, Shyam Prasad N wrote:
> A summary of the issue:
> With alpine linux containers (which uses the musl implementation of
> libc), the "rm -Rf" command could fail depending upon the dir size.
> The Linux cifs client filesystem behaviour is compared against ext4
> behaviour.
[...]
> Now the question is whether cifs.ko is doing anything wrong?
> @Steve French pointed me to this readdir documentation:
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir_r.html
> 
> If a file is removed from or added to the directory after the most
> recent call to opendir() or rewinddir(), whether a subsequent call to
> readdir() returns an entry for that file is unspecified.
> 
> So I guess the documents don't specify the behaviour in this case.

Or rather, your implementation of 'rm' is relying on unspecified
behaviour.  If it's doing rm -rf, it can keep calling readdir() [1]
but before it tries to unlink() the directory, it should rewinddir()
and see if it can find any more entries.  It shouldn't rely on the kernel
to fix this up.  ie:

	DIR *dirp = opendir(n);
	bool first = true;

	for (;;) {
		struct dirent *de = readdir(dirp);

		if (!de) {
			if first)
				break;
			rewinddir(dirp);
			continue;
		}
		first = false;
		unlink(de.d_name);
	}
	unlink(n);

... only with error checking and so on.

[1] Use readdir() rather than readdir_r() -- see the glibc 2.24+
documentation for details.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Linux-cifs readdir behaviour when dir modified
  2020-10-21  2:21 ` Matthew Wilcox
@ 2020-10-21  3:25   ` Shyam Prasad N
  0 siblings, 0 replies; 3+ messages in thread
From: Shyam Prasad N @ 2020-10-21  3:25 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: CIFS, linux-fsdevel, Steve French

Hi Matthew,

Thanks for the reply.
Sorry if it was unclear in my earlier email. I'm the engineer working
on cifs.ko, who's now trying to understand what's the "standard" way
of handling this which the VFS expects from the underlying filesystem.
And it sounds to me like there's no such standard way. I read this
codepath in a couple of popular filesystems, and each one seems to
have it's own way of handling this.

I wanted to reconfirm that the main issue is in the implementation of
the rm command on this distro, and the way it's using libc.

Regards,
Shyam

On Wed, Oct 21, 2020 at 7:51 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Oct 20, 2020 at 11:14:11AM +0530, Shyam Prasad N wrote:
> > A summary of the issue:
> > With alpine linux containers (which uses the musl implementation of
> > libc), the "rm -Rf" command could fail depending upon the dir size.
> > The Linux cifs client filesystem behaviour is compared against ext4
> > behaviour.
> [...]
> > Now the question is whether cifs.ko is doing anything wrong?
> > @Steve French pointed me to this readdir documentation:
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir_r.html
> >
> > If a file is removed from or added to the directory after the most
> > recent call to opendir() or rewinddir(), whether a subsequent call to
> > readdir() returns an entry for that file is unspecified.
> >
> > So I guess the documents don't specify the behaviour in this case.
>
> Or rather, your implementation of 'rm' is relying on unspecified
> behaviour.  If it's doing rm -rf, it can keep calling readdir() [1]
> but before it tries to unlink() the directory, it should rewinddir()
> and see if it can find any more entries.  It shouldn't rely on the kernel
> to fix this up.  ie:
>
>         DIR *dirp = opendir(n);
>         bool first = true;
>
>         for (;;) {
>                 struct dirent *de = readdir(dirp);
>
>                 if (!de) {
>                         if first)
>                                 break;
>                         rewinddir(dirp);
>                         continue;
>                 }
>                 first = false;
>                 unlink(de.d_name);
>         }
>         unlink(n);
>
> ... only with error checking and so on.
>
> [1] Use readdir() rather than readdir_r() -- see the glibc 2.24+
> documentation for details.



-- 
-Shyam

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-10-21  3:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-20  5:44 Linux-cifs readdir behaviour when dir modified Shyam Prasad N
2020-10-21  2:21 ` Matthew Wilcox
2020-10-21  3:25   ` Shyam Prasad N

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).