fuse readdir hangs kernel

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

fuse readdir hangs kernel

Helg Bredow
To replicate this bug.

1. Mount a fuse files system (e.g. ntfs-3g)
2. cd to a subdirectory (this is important)
3. run make (you don't need a makefile)

I've traced the cause to fuse_vnops.c line 704, which is in turn called by getcwd().

OpenBSD 6.2 GENERIC#103 amd64

--
Helg <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Martin Pieuchot
On 06/10/17(Fri) 12:26, Helg Bredow wrote:
> To replicate this bug.
>
> 1. Mount a fuse files system (e.g. ntfs-3g)
> 2. cd to a subdirectory (this is important)
> 3. run make (you don't need a makefile)
>
> I've traced the cause to fuse_vnops.c line 704, which is in turn called by getcwd().

How did you trace it?  Are you suggesting that the implied open/close
which is not implemented is the problem?

If that's the case, you might want to play with the (untested) diff
below:

Index: fuse/fuse_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.33
diff -u -p -r1.33 fuse_vnops.c
--- fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
+++ fuse/fuse_vnops.c 9 Oct 2017 12:42:13 -0000
@@ -680,7 +680,7 @@ fusefs_readdir(void *v)
  struct vnode *vp;
  struct proc *p;
  struct uio *uio;
- int error = 0, eofflag = 0;
+ int error = 0, eofflag = 0, diropen = 0;
 
  vp = ap->a_vp;
  uio = ap->a_uio;
@@ -699,9 +699,13 @@ fusefs_readdir(void *v)
  fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
 
  if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
- /* TODO open the file */
- fb_delete(fbuf);
- return (error);
+ error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY,
+    1, p);
+ if (error) {
+ fb_delete(fbuf);
+ break;
+ }
+ diropen = 1;
  }
  fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
  fbuf->fb_io_off = uio->uio_offset;
@@ -731,6 +735,9 @@ fusefs_readdir(void *v)
 
  if (!error && ap->a_eofflag != NULL)
  *ap->a_eofflag = eofflag;
+
+ if (diropen)
+ fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
 
  return (error);
 }

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
I initially noticed this on a modified kernel that returns EBADF when
the fusefs_readdir() code you modified is encountered. I then
downloaded the latest snapshot to see if I could replicate it and
noticed that it "hangs" the kernel (due to an endless loop). This bug
is not present in 6.1.

I used ktrace to see what system calls are being made and narrowed it
down to getcwd(3). I've noticed that there is no call to VOP_OPEN in
vfs_getcwd.c. Is that the correct behaviour?

I've tried your patch and had to modify it a bit so it compiles but it
doesn't solve the problem. The fusefs_file_open() call succeeds.
However, any call to getcwd(3) now fails with "No such file or
directory". I tested by creating a small test program that just makes
this function call.

On Mon, 9 Oct 2017 14:49:55 +0200
Martin Pieuchot <[hidden email]> wrote:

> On 06/10/17(Fri) 12:26, Helg Bredow wrote:
> > To replicate this bug.
> >
> > 1. Mount a fuse files system (e.g. ntfs-3g)
> > 2. cd to a subdirectory (this is important)
> > 3. run make (you don't need a makefile)
> >
> > I've traced the cause to fuse_vnops.c line 704, which is in turn called by getcwd().
>
> How did you trace it?  Are you suggesting that the implied open/close
> which is not implemented is the problem?
>
> If that's the case, you might want to play with the (untested) diff
> below:
>
> Index: fuse/fuse_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 fuse_vnops.c
> --- fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
> +++ fuse/fuse_vnops.c 9 Oct 2017 12:42:13 -0000
> @@ -680,7 +680,7 @@ fusefs_readdir(void *v)
>   struct vnode *vp;
>   struct proc *p;
>   struct uio *uio;
> - int error = 0, eofflag = 0;
> + int error = 0, eofflag = 0, diropen = 0;
>  
>   vp = ap->a_vp;
>   uio = ap->a_uio;
> @@ -699,9 +699,13 @@ fusefs_readdir(void *v)
>   fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
>  
>   if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> - /* TODO open the file */
> - fb_delete(fbuf);
> - return (error);
> + error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY,
> +    1, p);
> + if (error) {
> + fb_delete(fbuf);
> + break;
> + }
> + diropen = 1;
>   }
>   fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
>   fbuf->fb_io_off = uio->uio_offset;
> @@ -731,6 +735,9 @@ fusefs_readdir(void *v)
>  
>   if (!error && ap->a_eofflag != NULL)
>   *ap->a_eofflag = eofflag;
> +
> + if (diropen)
> + fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
>  
>   return (error);
>  }
>


--
Helg <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Martin Pieuchot
On 11/10/17(Wed) 11:25, Helg Bredow wrote:
> I initially noticed this on a modified kernel that returns EBADF when
> the fusefs_readdir() code you modified is encountered.

Yes, there's a bug in the current code.  A functionality is not
implemented and no error is returned.

What's the relevant part of ktrace/kdump with this modified kernel?

>                                                        I then
> downloaded the latest snapshot to see if I could replicate it and
> noticed that it "hangs" the kernel (due to an endless loop). This bug
> is not present in 6.1.

You should be able to enter ddb(4) and use the new "kill" command to
get out of the hang.

> I used ktrace to see what system calls are being made and narrowed it
> down to getcwd(3). I've noticed that there is no call to VOP_OPEN in
> vfs_getcwd.c. Is that the correct behaviour?

Yes it is.

> I've tried your patch and had to modify it a bit so it compiles but it
> doesn't solve the problem. The fusefs_file_open() call succeeds.
> However, any call to getcwd(3) now fails with "No such file or
> directory". I tested by creating a small test program that just makes
> this function call.

You need to figure out why vfs_getcwd_scandir() returns ENOENT.   What's
failing inside fuse_readdir() and why?

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
On Wed, 11 Oct 2017 13:53:26 +0200
Martin Pieuchot <[hidden email]> wrote:

> On 11/10/17(Wed) 11:25, Helg Bredow wrote:
> > I initially noticed this on a modified kernel that returns EBADF when
> > the fusefs_readdir() code you modified is encountered.
>
> Yes, there's a bug in the current code.  A functionality is not
> implemented and no error is returned.
>
> What's the relevant part of ktrace/kdump with this modified kernel?
>
> >                                                        I then
> > downloaded the latest snapshot to see if I could replicate it and
> > noticed that it "hangs" the kernel (due to an endless loop). This bug
> > is not present in 6.1.
>
> You should be able to enter ddb(4) and use the new "kill" command to
> get out of the hang.
>
> > I used ktrace to see what system calls are being made and narrowed it
> > down to getcwd(3). I've noticed that there is no call to VOP_OPEN in
> > vfs_getcwd.c. Is that the correct behaviour?
>
> Yes it is.
>
> > I've tried your patch and had to modify it a bit so it compiles but it
> > doesn't solve the problem. The fusefs_file_open() call succeeds.
> > However, any call to getcwd(3) now fails with "No such file or
> > directory". I tested by creating a small test program that just makes
> > this function call.
>
> You need to figure out why vfs_getcwd_scandir() returns ENOENT.   What's
> failing inside fuse_readdir() and why?
>

The reason is that ifuse_fill_readdir() sets d_fileno to the ino value returned by the file system but fuse otherwise uses its own generated ino. The Linux version of fuse has the use_ino and readdir_ino options to modify this behaviour. OpenBSD currently behaves as if the use_ino option was not set. i.e. don't use the file system ino values.

I've attempted to fix this by calling get_vn_by_name_and_parent() inside the filler function and it works some of the time but not others. Any suggestions?

--
Helg <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
On Fri, 13 Oct 2017 12:54:31 +0000
Helg Bredow <[hidden email]> wrote:

> On Wed, 11 Oct 2017 13:53:26 +0200
> Martin Pieuchot <[hidden email]> wrote:
>
> > On 11/10/17(Wed) 11:25, Helg Bredow wrote:
> > > I initially noticed this on a modified kernel that returns EBADF when
> > > the fusefs_readdir() code you modified is encountered.
> >
> > Yes, there's a bug in the current code.  A functionality is not
> > implemented and no error is returned.
> >
> > What's the relevant part of ktrace/kdump with this modified kernel?
> >
> > >                                                        I then
> > > downloaded the latest snapshot to see if I could replicate it and
> > > noticed that it "hangs" the kernel (due to an endless loop). This bug
> > > is not present in 6.1.
> >
> > You should be able to enter ddb(4) and use the new "kill" command to
> > get out of the hang.
> >
> > > I used ktrace to see what system calls are being made and narrowed it
> > > down to getcwd(3). I've noticed that there is no call to VOP_OPEN in
> > > vfs_getcwd.c. Is that the correct behaviour?
> >
> > Yes it is.
> >
> > > I've tried your patch and had to modify it a bit so it compiles but it
> > > doesn't solve the problem. The fusefs_file_open() call succeeds.
> > > However, any call to getcwd(3) now fails with "No such file or
> > > directory". I tested by creating a small test program that just makes
> > > this function call.
> >
> > You need to figure out why vfs_getcwd_scandir() returns ENOENT.   What's
> > failing inside fuse_readdir() and why?
> >
>
> The reason is that ifuse_fill_readdir() sets d_fileno to the ino value returned by the file system but fuse otherwise uses its own generated ino. The Linux version of fuse has the use_ino and readdir_ino options to modify this behaviour. OpenBSD currently behaves as if the use_ino option was not set. i.e. don't use the file system ino values.
>
> I've attempted to fix this by calling get_vn_by_name_and_parent() inside the filler function and it works some of the time but not others. Any suggestions?
>

Just in case anyone else is working on this. I've found the cause and am working on a patch.

--
Helg <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
On Sun, 15 Oct 2017 14:22:06 +0000
Helg Bredow <[hidden email]> wrote:

> On Fri, 13 Oct 2017 12:54:31 +0000
> Helg Bredow <[hidden email]> wrote:
>
> > On Wed, 11 Oct 2017 13:53:26 +0200
> > Martin Pieuchot <[hidden email]> wrote:
> >
> > > On 11/10/17(Wed) 11:25, Helg Bredow wrote:
> > > > I initially noticed this on a modified kernel that returns EBADF when
> > > > the fusefs_readdir() code you modified is encountered.
> > >
> > > Yes, there's a bug in the current code.  A functionality is not
> > > implemented and no error is returned.
> > >
> > > What's the relevant part of ktrace/kdump with this modified kernel?
> > >
> > > >                                                        I then
> > > > downloaded the latest snapshot to see if I could replicate it and
> > > > noticed that it "hangs" the kernel (due to an endless loop). This bug
> > > > is not present in 6.1.
> > >
> > > You should be able to enter ddb(4) and use the new "kill" command to
> > > get out of the hang.
> > >
> > > > I used ktrace to see what system calls are being made and narrowed it
> > > > down to getcwd(3). I've noticed that there is no call to VOP_OPEN in
> > > > vfs_getcwd.c. Is that the correct behaviour?
> > >
> > > Yes it is.
> > >
> > > > I've tried your patch and had to modify it a bit so it compiles but it
> > > > doesn't solve the problem. The fusefs_file_open() call succeeds.
> > > > However, any call to getcwd(3) now fails with "No such file or
> > > > directory". I tested by creating a small test program that just makes
> > > > this function call.
> > >
> > > You need to figure out why vfs_getcwd_scandir() returns ENOENT.   What's
> > > failing inside fuse_readdir() and why?
> > >
> >
> > The reason is that ifuse_fill_readdir() sets d_fileno to the ino value returned by the file system but fuse otherwise uses its own generated ino. The Linux version of fuse has the use_ino and readdir_ino options to modify this behaviour. OpenBSD currently behaves as if the use_ino option was not set. i.e. don't use the file system ino values.
> >
> > I've attempted to fix this by calling get_vn_by_name_and_parent() inside the filler function and it works some of the time but not others. Any suggestions?
> >
>
> Just in case anyone else is working on this. I've found the cause and am working on a patch.
>
> --
> Helg <[hidden email]>

Patch is below.


Index: lib/libfuse/fuse.h
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse.h,v
retrieving revision 1.12
diff -u -p -r1.12 fuse.h
--- lib/libfuse/fuse.h 20 Jan 2014 15:01:59 -0000 1.12
+++ lib/libfuse/fuse.h 16 Oct 2017 14:21:06 -0000
@@ -89,6 +89,7 @@ typedef int (*fuse_fill_dir_t)(void *, c
     off_t);
 
 typedef struct fuse_dirhandle {
+ struct fuse *fuse;
  fuse_fill_dir_t filler;
  void *buf;
  int filled;
Index: lib/libfuse/fuse_ops.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
retrieving revision 1.26
diff -u -p -r1.26 fuse_ops.c
--- lib/libfuse/fuse_ops.c 7 Sep 2016 17:53:35 -0000 1.26
+++ lib/libfuse/fuse_ops.c 16 Oct 2017 14:21:06 -0000
@@ -217,17 +217,23 @@ static int
 ifuse_fill_readdir(void *dh, const char *name, const struct stat *stbuf,
     off_t off)
 {
+ struct fuse *f;
  struct fuse_dirhandle *fd = dh;
+ struct fuse_vnode *v;
  struct fusebuf *fbuf;
  struct dirent *dir;
  uint32_t namelen;
  uint32_t len;
 
+ f = fd->fuse;
  fbuf = fd->buf;
  namelen = strnlen(name, MAXNAMLEN);
  len = GENERIC_DIRSIZ(namelen);
 
- if (fd->full || (fbuf->fb_len + len > fd->size)) {
+ if (fd->full)
+ return (0);
+
+ if (fbuf->fb_len + len > fd->size) {
  fd->full = 1;
  return (0);
  }
@@ -242,13 +248,21 @@ ifuse_fill_readdir(void *dh, const char
  if (off)
  fd->filled = 0;
 
- if (stbuf) {
- dir->d_fileno = stbuf->st_ino;
+ /* TODO Add support for use_ino and readdir_ino */
+ v = get_vn_by_name_and_parent(f, (uint8_t *)name, fbuf->fb_ino);
+ if (v == NULL) {
+ if (strcmp(name, ".") == 0)
+ dir->d_fileno = fbuf->fb_ino;
+ else
+ dir->d_fileno = 0xffffffff;
+ } else
+ dir->d_fileno = v->ino;
+
+ if (stbuf)
  dir->d_type = IFTODT(stbuf->st_mode);
- } else {
- dir->d_fileno = 0xffffffff;
+ else
  dir->d_type = DT_UNKNOWN;
- }
+
  dir->d_reclen = len;
  dir->d_off = off + len; /* XXX */
  strlcpy(dir->d_name, name, sizeof(dir->d_name));
@@ -295,7 +309,7 @@ ifuse_ops_readdir(struct fuse *f, struct
  ffi.fh = fbuf->fb_io_fd;
  offset = fbuf->fb_io_off;
  size = fbuf->fb_io_len;
- startsave = 0;
+ startsave = offset;
 
  fbuf->fb_dat = calloc(1, size);
 
@@ -319,6 +333,8 @@ ifuse_ops_readdir(struct fuse *f, struct
  vn->fd->size = size;
  vn->fd->off = offset;
  vn->fd->idx = 0;
+ vn->fd->fuse = f;
+ vn->fd->start = offset;
  startsave = vn->fd->start;
 
  realname = build_realname(f, vn->ino);
@@ -345,7 +361,8 @@ ifuse_ops_readdir(struct fuse *f, struct
  if (fbuf->fb_err) {
  fbuf->fb_len = 0;
  vn->fd->filled = 1;
- }
+ } else if (vn->fd->full && fbuf->fb_len == 0)
+ fbuf->fb_err = -ENOBUFS;
 
  if (fbuf->fb_len == 0)
  free(fbuf->fb_dat);
Index: sys/miscfs/fuse/fuse_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.33
diff -u -p -r1.33 fuse_vnops.c
--- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
+++ sys/miscfs/fuse/fuse_vnops.c 16 Oct 2017 14:21:17 -0000
@@ -680,7 +680,7 @@ fusefs_readdir(void *v)
  struct vnode *vp;
  struct proc *p;
  struct uio *uio;
- int error = 0, eofflag = 0;
+ int error = 0, eofflag = 0, diropen = 0;
 
  vp = ap->a_vp;
  uio = ap->a_uio;
@@ -695,14 +695,18 @@ fusefs_readdir(void *v)
  if (uio->uio_resid < sizeof(struct dirent))
  return (EINVAL);
 
+ if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
+ error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
+ if (error) {
+ fb_delete(fbuf);
+ goto out;
+ }
+ diropen = 1;
+ }
+
  while (uio->uio_resid > 0) {
  fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
 
- if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
- /* TODO open the file */
- fb_delete(fbuf);
- return (error);
- }
  fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
  fbuf->fb_io_off = uio->uio_offset;
  fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
@@ -710,6 +714,11 @@ fusefs_readdir(void *v)
  error = fb_queue(fmp->dev, fbuf);
 
  if (error) {
+ /* Not a real error, dirent was larger than residual
+     space left in buffer. Return as normal. */
+ if (error == ENOBUFS && fbuf->fb_len == 0)
+ error = 0;
+
  fb_delete(fbuf);
  break;
  }
@@ -732,6 +741,10 @@ fusefs_readdir(void *v)
  if (!error && ap->a_eofflag != NULL)
  *ap->a_eofflag = eofflag;
 
+ if (diropen)
+ fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
+
+out:
  return (error);
 }
 


Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Martin Pieuchot
On 16/10/17(Mon) 14:37, Helg Bredow wrote:
> [...]
> Patch is below.

Do you mind explaining the problem and the solution you implemented?

Some comments below:

> Index: lib/libfuse/fuse.h
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 fuse.h
> --- lib/libfuse/fuse.h 20 Jan 2014 15:01:59 -0000 1.12
> +++ lib/libfuse/fuse.h 16 Oct 2017 14:21:06 -0000
> @@ -89,6 +89,7 @@ typedef int (*fuse_fill_dir_t)(void *, c
>      off_t);
>  
>  typedef struct fuse_dirhandle {
> + struct fuse *fuse;

Are you sure you can save this across calls?

>   fuse_fill_dir_t filler;
>   void *buf;
>   int filled;
> Index: lib/libfuse/fuse_ops.c
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 fuse_ops.c
> --- lib/libfuse/fuse_ops.c 7 Sep 2016 17:53:35 -0000 1.26
> +++ lib/libfuse/fuse_ops.c 16 Oct 2017 14:21:06 -0000
> @@ -217,17 +217,23 @@ static int
>  ifuse_fill_readdir(void *dh, const char *name, const struct stat *stbuf,
>      off_t off)
>  {
> + struct fuse *f;
>   struct fuse_dirhandle *fd = dh;
> + struct fuse_vnode *v;
>   struct fusebuf *fbuf;
>   struct dirent *dir;
>   uint32_t namelen;
>   uint32_t len;
>  
> + f = fd->fuse;
>   fbuf = fd->buf;
>   namelen = strnlen(name, MAXNAMLEN);
>   len = GENERIC_DIRSIZ(namelen);
>  
> - if (fd->full || (fbuf->fb_len + len > fd->size)) {
> + if (fd->full)
> + return (0);
> +
> + if (fbuf->fb_len + len > fd->size) {
>   fd->full = 1;
>   return (0);

Why is this chunk necessary?  Is it part of the fix?

>   }
> @@ -242,13 +248,21 @@ ifuse_fill_readdir(void *dh, const char
>   if (off)
>   fd->filled = 0;
>  
> - if (stbuf) {
> - dir->d_fileno = stbuf->st_ino;
> + /* TODO Add support for use_ino and readdir_ino */
> + v = get_vn_by_name_and_parent(f, (uint8_t *)name, fbuf->fb_ino);
> + if (v == NULL) {
> + if (strcmp(name, ".") == 0)
> + dir->d_fileno = fbuf->fb_ino;
> + else
> + dir->d_fileno = 0xffffffff;
> + } else
> + dir->d_fileno = v->ino;
> +
> + if (stbuf)
>   dir->d_type = IFTODT(stbuf->st_mode);
> - } else {
> - dir->d_fileno = 0xffffffff;
> + else
>   dir->d_type = DT_UNKNOWN;
> - }
> +
>   dir->d_reclen = len;
>   dir->d_off = off + len; /* XXX */
>   strlcpy(dir->d_name, name, sizeof(dir->d_name));
> @@ -295,7 +309,7 @@ ifuse_ops_readdir(struct fuse *f, struct
>   ffi.fh = fbuf->fb_io_fd;
>   offset = fbuf->fb_io_off;
>   size = fbuf->fb_io_len;
> - startsave = 0;
> + startsave = offset;
>  
>   fbuf->fb_dat = calloc(1, size);
>  
> @@ -319,6 +333,8 @@ ifuse_ops_readdir(struct fuse *f, struct
>   vn->fd->size = size;
>   vn->fd->off = offset;
>   vn->fd->idx = 0;
> + vn->fd->fuse = f;
> + vn->fd->start = offset;
>   startsave = vn->fd->start;
>  
>   realname = build_realname(f, vn->ino);
> @@ -345,7 +361,8 @@ ifuse_ops_readdir(struct fuse *f, struct
>   if (fbuf->fb_err) {
>   fbuf->fb_len = 0;
>   vn->fd->filled = 1;
> - }
> + } else if (vn->fd->full && fbuf->fb_len == 0)
> + fbuf->fb_err = -ENOBUFS;
>  
>   if (fbuf->fb_len == 0)
>   free(fbuf->fb_dat);
> Index: sys/miscfs/fuse/fuse_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 fuse_vnops.c
> --- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
> +++ sys/miscfs/fuse/fuse_vnops.c 16 Oct 2017 14:21:17 -0000
> @@ -680,7 +680,7 @@ fusefs_readdir(void *v)
>   struct vnode *vp;
>   struct proc *p;
>   struct uio *uio;
> - int error = 0, eofflag = 0;
> + int error = 0, eofflag = 0, diropen = 0;
>  
>   vp = ap->a_vp;
>   uio = ap->a_uio;
> @@ -695,14 +695,18 @@ fusefs_readdir(void *v)
>   if (uio->uio_resid < sizeof(struct dirent))
>   return (EINVAL);
>  
> + if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> + error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
> + if (error) {
> + fb_delete(fbuf);
> + goto out;

You can use return (error) here and remove the goto.

> + }
> + diropen = 1;
> + }
> +
>   while (uio->uio_resid > 0) {
>   fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
>  
> - if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> - /* TODO open the file */
> - fb_delete(fbuf);
> - return (error);
> - }
>   fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
>   fbuf->fb_io_off = uio->uio_offset;
>   fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
> @@ -710,6 +714,11 @@ fusefs_readdir(void *v)
>   error = fb_queue(fmp->dev, fbuf);
>  
>   if (error) {
> + /* Not a real error, dirent was larger than residual
> +     space left in buffer. Return as normal. */

s/Not a real error,//

s/return as normal.//

And look a style(9) for the comment formatting.

> + if (error == ENOBUFS && fbuf->fb_len == 0)
> + error = 0;
> +
>   fb_delete(fbuf);
>   break;
>   }
> @@ -732,6 +741,10 @@ fusefs_readdir(void *v)
>   if (!error && ap->a_eofflag != NULL)
>   *ap->a_eofflag = eofflag;
>  
> + if (diropen)
> + fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
> +
> +out:
>   return (error);
>  }
>  
>
>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
On Wed, 18 Oct 2017 10:47:59 +0200
Martin Pieuchot <[hidden email]> wrote:

> On 16/10/17(Mon) 14:37, Helg Bredow wrote:
> > [...]
> > Patch is below.
>
> Do you mind explaining the problem and the solution you implemented?
>

There is a regression introduced in 6.2 that can cause a call to getcwd(3) to result in an endless loop. fusefs_readdir() has a TODO that returns success even if a directory is not open, so getcwd(3) keeps calling it to fetch more dirents but none ever get supplied. This patch opens and closes the directory if it's not already open. The check for an invalid file handle was inside the loop but I've moved it outside since it's not going to become invalid once it's been opened.

This doesn't resolve the problem however. This is because ifuse_ops_readdir() does not support being called across multiple calls to VOP_READDIR(9). This occurs if the buffer is too small to contain all the entries and getcwd(3) only supplies a 512 byte buffer. If the current directory is not returned in the first call to VOP_READDIR(9) then getcwd(3) will continue to call it but the eofflag is never set. ifuse_ops_readdir() now correctly starts scanning at the next dirent even across multiple VOP_READDIR(9) calls.

In addition, if the entries don't evenly fit into the buffer then fusefs_readdir() would prematurely detect that the entire directory had been scanned and set the eofflag. This fix handles this condition so the entire directory is always scanned.

ifuse_fill_readdir() returns the ino of the file system instead of its own ino, which it returns from other calls such as stat(2). getcwd(3) compares these inos when scanning the directory and will never find a match since they are sourced from different file systems. fuse has the use_ino option to allow a file system to use its own inos but this is not implemented in OpenBSD yet. This patch forces the fuse ino to be used in ifuse_fill_readdir(). This is still only paritally implemented though, it will only set a valid ino if it can find the file or directory in the name cache. This is sufficient for getcwd(3) since the parent directory will always be in the cache.

Thanks for your feedback mpi@; I've made the changes you suggested. It's safe to store the fuse struct across calls since this is returned from fuse_setup() and never changes. Without it, I'm not sure how I would implement a solution since you need this in the filler function to access the name dictionary.


Index: lib/libfuse/fuse.h
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse.h,v
retrieving revision 1.12
diff -u -p -r1.12 fuse.h
--- lib/libfuse/fuse.h 20 Jan 2014 15:01:59 -0000 1.12
+++ lib/libfuse/fuse.h 18 Oct 2017 13:24:53 -0000
@@ -89,6 +89,7 @@ typedef int (*fuse_fill_dir_t)(void *, c
     off_t);
 
 typedef struct fuse_dirhandle {
+ struct fuse *fuse;
  fuse_fill_dir_t filler;
  void *buf;
  int filled;
Index: lib/libfuse/fuse_ops.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
retrieving revision 1.26
diff -u -p -r1.26 fuse_ops.c
--- lib/libfuse/fuse_ops.c 7 Sep 2016 17:53:35 -0000 1.26
+++ lib/libfuse/fuse_ops.c 18 Oct 2017 13:24:53 -0000
@@ -217,17 +217,20 @@ static int
 ifuse_fill_readdir(void *dh, const char *name, const struct stat *stbuf,
     off_t off)
 {
+ struct fuse *f;
  struct fuse_dirhandle *fd = dh;
+ struct fuse_vnode *v;
  struct fusebuf *fbuf;
  struct dirent *dir;
  uint32_t namelen;
  uint32_t len;
 
+ f = fd->fuse;
  fbuf = fd->buf;
  namelen = strnlen(name, MAXNAMLEN);
  len = GENERIC_DIRSIZ(namelen);
 
- if (fd->full || (fbuf->fb_len + len > fd->size)) {
+ if (fd->full || fbuf->fb_len + len > fd->size) {
  fd->full = 1;
  return (0);
  }
@@ -242,13 +245,21 @@ ifuse_fill_readdir(void *dh, const char
  if (off)
  fd->filled = 0;
 
- if (stbuf) {
- dir->d_fileno = stbuf->st_ino;
+ /* TODO Add support for use_ino and readdir_ino */
+ v = get_vn_by_name_and_parent(f, (uint8_t *)name, fbuf->fb_ino);
+ if (v == NULL) {
+ if (strcmp(name, ".") == 0)
+ dir->d_fileno = fbuf->fb_ino;
+ else
+ dir->d_fileno = 0xffffffff;
+ } else
+ dir->d_fileno = v->ino;
+
+ if (stbuf)
  dir->d_type = IFTODT(stbuf->st_mode);
- } else {
- dir->d_fileno = 0xffffffff;
+ else
  dir->d_type = DT_UNKNOWN;
- }
+
  dir->d_reclen = len;
  dir->d_off = off + len; /* XXX */
  strlcpy(dir->d_name, name, sizeof(dir->d_name));
@@ -295,7 +306,7 @@ ifuse_ops_readdir(struct fuse *f, struct
  ffi.fh = fbuf->fb_io_fd;
  offset = fbuf->fb_io_off;
  size = fbuf->fb_io_len;
- startsave = 0;
+ startsave = offset;
 
  fbuf->fb_dat = calloc(1, size);
 
@@ -319,6 +330,8 @@ ifuse_ops_readdir(struct fuse *f, struct
  vn->fd->size = size;
  vn->fd->off = offset;
  vn->fd->idx = 0;
+ vn->fd->fuse = f;
+ vn->fd->start = offset;
  startsave = vn->fd->start;
 
  realname = build_realname(f, vn->ino);
@@ -345,7 +358,8 @@ ifuse_ops_readdir(struct fuse *f, struct
  if (fbuf->fb_err) {
  fbuf->fb_len = 0;
  vn->fd->filled = 1;
- }
+ } else if (vn->fd->full && fbuf->fb_len == 0)
+ fbuf->fb_err = -ENOBUFS;
 
  if (fbuf->fb_len == 0)
  free(fbuf->fb_dat);
Index: sys/miscfs/fuse/fuse_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.33
diff -u -p -r1.33 fuse_vnops.c
--- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
+++ sys/miscfs/fuse/fuse_vnops.c 18 Oct 2017 13:25:04 -0000
@@ -680,7 +680,7 @@ fusefs_readdir(void *v)
  struct vnode *vp;
  struct proc *p;
  struct uio *uio;
- int error = 0, eofflag = 0;
+ int error = 0, eofflag = 0, diropen = 0;
 
  vp = ap->a_vp;
  uio = ap->a_uio;
@@ -695,14 +695,18 @@ fusefs_readdir(void *v)
  if (uio->uio_resid < sizeof(struct dirent))
  return (EINVAL);
 
- while (uio->uio_resid > 0) {
- fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
-
- if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
- /* TODO open the file */
+ if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
+ error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
+ if (error) {
  fb_delete(fbuf);
  return (error);
  }
+ diropen = 1;
+ }
+
+ while (uio->uio_resid > 0) {
+ fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
+
  fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
  fbuf->fb_io_off = uio->uio_offset;
  fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
@@ -710,6 +714,13 @@ fusefs_readdir(void *v)
  error = fb_queue(fmp->dev, fbuf);
 
  if (error) {
+ /*
+ * dirent was larger than residual space left in
+ * buffer.
+ */
+ if (error == ENOBUFS && fbuf->fb_len == 0)
+ error = 0;
+
  fb_delete(fbuf);
  break;
  }
@@ -731,6 +742,9 @@ fusefs_readdir(void *v)
 
  if (!error && ap->a_eofflag != NULL)
  *ap->a_eofflag = eofflag;
+
+ if (diropen)
+ fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
 
  return (error);
 }

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
On Wed, 18 Oct 2017 14:19:31 +0000
Helg Bredow <[hidden email]> wrote:

> On Wed, 18 Oct 2017 10:47:59 +0200
> Martin Pieuchot <[hidden email]> wrote:
>
> > On 16/10/17(Mon) 14:37, Helg Bredow wrote:
> > > [...]
> > > Patch is below.
> >
> > Do you mind explaining the problem and the solution you implemented?
> >
>
> There is a regression introduced in 6.2 that can cause a call to getcwd(3) to result in an endless loop. fusefs_readdir() has a TODO that returns success even if a directory is not open, so getcwd(3) keeps calling it to fetch more dirents but none ever get supplied. This patch opens and closes the directory if it's not already open. The check for an invalid file handle was inside the loop but I've moved it outside since it's not going to become invalid once it's been opened.
>
> This doesn't resolve the problem however. This is because ifuse_ops_readdir() does not support being called across multiple calls to VOP_READDIR(9). This occurs if the buffer is too small to contain all the entries and getcwd(3) only supplies a 512 byte buffer. If the current directory is not returned in the first call to VOP_READDIR(9) then getcwd(3) will continue to call it but the eofflag is never set. ifuse_ops_readdir() now correctly starts scanning at the next dirent even across multiple VOP_READDIR(9) calls.
>
> In addition, if the entries don't evenly fit into the buffer then fusefs_readdir() would prematurely detect that the entire directory had been scanned and set the eofflag. This fix handles this condition so the entire directory is always scanned.
>
> ifuse_fill_readdir() returns the ino of the file system instead of its own ino, which it returns from other calls such as stat(2). getcwd(3) compares these inos when scanning the directory and will never find a match since they are sourced from different file systems. fuse has the use_ino option to allow a file system to use its own inos but this is not implemented in OpenBSD yet. This patch forces the fuse ino to be used in ifuse_fill_readdir(). This is still only paritally implemented though, it will only set a valid ino if it can find the file or directory in the name cache. This is sufficient for getcwd(3) since the parent directory will always be in the cache.
>
> Thanks for your feedback mpi@; I've made the changes you suggested. It's safe to store the fuse struct across calls since this is returned from fuse_setup() and never changes. Without it, I'm not sure how I would implement a solution since you need this in the filler function to access the name dictionary.
>
>
> Index: lib/libfuse/fuse.h
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 fuse.h
> --- lib/libfuse/fuse.h 20 Jan 2014 15:01:59 -0000 1.12
> +++ lib/libfuse/fuse.h 18 Oct 2017 13:24:53 -0000
> @@ -89,6 +89,7 @@ typedef int (*fuse_fill_dir_t)(void *, c
>      off_t);
>  
>  typedef struct fuse_dirhandle {
> + struct fuse *fuse;
>   fuse_fill_dir_t filler;
>   void *buf;
>   int filled;
> Index: lib/libfuse/fuse_ops.c
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 fuse_ops.c
> --- lib/libfuse/fuse_ops.c 7 Sep 2016 17:53:35 -0000 1.26
> +++ lib/libfuse/fuse_ops.c 18 Oct 2017 13:24:53 -0000
> @@ -217,17 +217,20 @@ static int
>  ifuse_fill_readdir(void *dh, const char *name, const struct stat *stbuf,
>      off_t off)
>  {
> + struct fuse *f;
>   struct fuse_dirhandle *fd = dh;
> + struct fuse_vnode *v;
>   struct fusebuf *fbuf;
>   struct dirent *dir;
>   uint32_t namelen;
>   uint32_t len;
>  
> + f = fd->fuse;
>   fbuf = fd->buf;
>   namelen = strnlen(name, MAXNAMLEN);
>   len = GENERIC_DIRSIZ(namelen);
>  
> - if (fd->full || (fbuf->fb_len + len > fd->size)) {
> + if (fd->full || fbuf->fb_len + len > fd->size) {
>   fd->full = 1;
>   return (0);
>   }
> @@ -242,13 +245,21 @@ ifuse_fill_readdir(void *dh, const char
>   if (off)
>   fd->filled = 0;
>  
> - if (stbuf) {
> - dir->d_fileno = stbuf->st_ino;
> + /* TODO Add support for use_ino and readdir_ino */
> + v = get_vn_by_name_and_parent(f, (uint8_t *)name, fbuf->fb_ino);
> + if (v == NULL) {
> + if (strcmp(name, ".") == 0)
> + dir->d_fileno = fbuf->fb_ino;
> + else
> + dir->d_fileno = 0xffffffff;
> + } else
> + dir->d_fileno = v->ino;
> +
> + if (stbuf)
>   dir->d_type = IFTODT(stbuf->st_mode);
> - } else {
> - dir->d_fileno = 0xffffffff;
> + else
>   dir->d_type = DT_UNKNOWN;
> - }
> +
>   dir->d_reclen = len;
>   dir->d_off = off + len; /* XXX */
>   strlcpy(dir->d_name, name, sizeof(dir->d_name));
> @@ -295,7 +306,7 @@ ifuse_ops_readdir(struct fuse *f, struct
>   ffi.fh = fbuf->fb_io_fd;
>   offset = fbuf->fb_io_off;
>   size = fbuf->fb_io_len;
> - startsave = 0;
> + startsave = offset;
>  
>   fbuf->fb_dat = calloc(1, size);
>  
> @@ -319,6 +330,8 @@ ifuse_ops_readdir(struct fuse *f, struct
>   vn->fd->size = size;
>   vn->fd->off = offset;
>   vn->fd->idx = 0;
> + vn->fd->fuse = f;
> + vn->fd->start = offset;
>   startsave = vn->fd->start;
>  
>   realname = build_realname(f, vn->ino);
> @@ -345,7 +358,8 @@ ifuse_ops_readdir(struct fuse *f, struct
>   if (fbuf->fb_err) {
>   fbuf->fb_len = 0;
>   vn->fd->filled = 1;
> - }
> + } else if (vn->fd->full && fbuf->fb_len == 0)
> + fbuf->fb_err = -ENOBUFS;
>  
>   if (fbuf->fb_len == 0)
>   free(fbuf->fb_dat);
> Index: sys/miscfs/fuse/fuse_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 fuse_vnops.c
> --- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
> +++ sys/miscfs/fuse/fuse_vnops.c 18 Oct 2017 13:25:04 -0000
> @@ -680,7 +680,7 @@ fusefs_readdir(void *v)
>   struct vnode *vp;
>   struct proc *p;
>   struct uio *uio;
> - int error = 0, eofflag = 0;
> + int error = 0, eofflag = 0, diropen = 0;
>  
>   vp = ap->a_vp;
>   uio = ap->a_uio;
> @@ -695,14 +695,18 @@ fusefs_readdir(void *v)
>   if (uio->uio_resid < sizeof(struct dirent))
>   return (EINVAL);
>  
> - while (uio->uio_resid > 0) {
> - fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
> -
> - if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> - /* TODO open the file */
> + if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> + error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
> + if (error) {
>   fb_delete(fbuf);
>   return (error);
>   }
> + diropen = 1;
> + }
> +
> + while (uio->uio_resid > 0) {
> + fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
> +
>   fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
>   fbuf->fb_io_off = uio->uio_offset;
>   fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
> @@ -710,6 +714,13 @@ fusefs_readdir(void *v)
>   error = fb_queue(fmp->dev, fbuf);
>  
>   if (error) {
> + /*
> + * dirent was larger than residual space left in
> + * buffer.
> + */
> + if (error == ENOBUFS && fbuf->fb_len == 0)
> + error = 0;
> +
>   fb_delete(fbuf);
>   break;
>   }
> @@ -731,6 +742,9 @@ fusefs_readdir(void *v)
>  
>   if (!error && ap->a_eofflag != NULL)
>   *ap->a_eofflag = eofflag;
> +
> + if (diropen)
> + fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
>  
>   return (error);
>  }
>

Does this explain the problem and solution or do you need more information? Is there any interest from the developers in patches to improve fuse? Let me know and I'll continue to work on it.

--
Helg <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Martin Pieuchot
On 22/10/17(Sun) 01:21, Helg Bredow wrote:
> [...]
> Does this explain the problem and solution or do you need more information? Is there any interest from the developers in patches to improve fuse? Let me know and I'll continue to work on it.

There's a lot of interest to improve fuse.  I just need to take some time
to reviews our emails and diffs.  I'll come back to you in a  couple of
days if nobody beats me.

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Martin Pieuchot
In reply to this post by Helg Bredow
On 18/10/17(Wed) 14:19, Helg Bredow wrote:

> On Wed, 18 Oct 2017 10:47:59 +0200
> Martin Pieuchot <[hidden email]> wrote:
>
> > On 16/10/17(Mon) 14:37, Helg Bredow wrote:
> > > [...]
> > > Patch is below.
> >
> > Do you mind explaining the problem and the solution you implemented?
> >
>
> There is a regression introduced in 6.2 that can cause a call to getcwd(3) to result in an endless loop. fusefs_readdir() has a TODO that returns success even if a directory is not open, so getcwd(3) keeps calling it to fetch more dirents but none ever get supplied. This patch opens and closes the directory if it's not already open. The check for an invalid file handle was inside the loop but I've moved it outside since it's not going to become invalid once it's been opened.

Nice.  You can remove a call to fb_delete() in the first error path.

> This doesn't resolve the problem however. This is because ifuse_ops_readdir() does not support being called across multiple calls to VOP_READDIR(9). This occurs if the buffer is too small to contain all the entries and getcwd(3) only supplies a 512 byte buffer. If the current directory is not returned in the first call to VOP_READDIR(9) then getcwd(3) will continue to call it but the eofflag is never set. ifuse_ops_readdir() now correctly starts scanning at the next dirent even across multiple VOP_READDIR(9) calls.

Why did you decide to put the logic in the library?  Now the library
needs to remember some "state".

Would it be possible to keep a dumb ifuse_ops_readdir() and make the
kernel ask for the next dirent?  How often does that happen in
practise?  Only when strlen(dirent) > 512?

> In addition, if the entries don't evenly fit into the buffer then fusefs_readdir() would prematurely detect that the entire directory had been scanned and set the eofflag. This fix handles this condition so the entire directory is always scanned.

That's good.

> ifuse_fill_readdir() returns the ino of the file system instead of its own ino, which it returns from other calls such as stat(2). getcwd(3) compares these inos when scanning the directory and will never find a match since they are sourced from different file systems. fuse has the use_ino option to allow a file system to use its own inos but this is not implemented in OpenBSD yet. This patch forces the fuse ino to be used in ifuse_fill_readdir(). This is still only paritally implemented though, it will only set a valid ino if it can find the file or directory in the name cache. This is sufficient for getcwd(3) since the parent directory will always be in the cache.

How much work would it be to support the `use_ino' option?

> Index: sys/miscfs/fuse/fuse_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 fuse_vnops.c
> --- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
> +++ sys/miscfs/fuse/fuse_vnops.c 18 Oct 2017 13:25:04 -0000
> @@ -680,7 +680,7 @@ fusefs_readdir(void *v)
>   struct vnode *vp;
>   struct proc *p;
>   struct uio *uio;
> - int error = 0, eofflag = 0;
> + int error = 0, eofflag = 0, diropen = 0;
>  
>   vp = ap->a_vp;
>   uio = ap->a_uio;
> @@ -695,14 +695,18 @@ fusefs_readdir(void *v)
>   if (uio->uio_resid < sizeof(struct dirent))
>   return (EINVAL);
>  
> - while (uio->uio_resid > 0) {
> - fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
> -
> - if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> - /* TODO open the file */
> + if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> + error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
> + if (error) {
>   fb_delete(fbuf);

No need for fb_delete() here.

>   return (error);
>   }
> + diropen = 1;
> + }
> +
> + while (uio->uio_resid > 0) {
> + fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
> +
>   fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
>   fbuf->fb_io_off = uio->uio_offset;
>   fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
> @@ -710,6 +714,13 @@ fusefs_readdir(void *v)
>   error = fb_queue(fmp->dev, fbuf);
>  
>   if (error) {
> + /*
> + * dirent was larger than residual space left in
> + * buffer.
> + */
> + if (error == ENOBUFS && fbuf->fb_len == 0)
> + error = 0;
> +
>   fb_delete(fbuf);
>   break;
>   }
> @@ -731,6 +742,9 @@ fusefs_readdir(void *v)
>  
>   if (!error && ap->a_eofflag != NULL)
>   *ap->a_eofflag = eofflag;
> +
> + if (diropen)
> + fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
>  
>   return (error);
>  }
>

Reply | Threaded
Open this post in threaded view
|

Re: fuse readdir hangs kernel

Helg Bredow
On Sat, 28 Oct 2017 09:50:35 +0200
Martin Pieuchot <[hidden email]> wrote:

> On 18/10/17(Wed) 14:19, Helg Bredow wrote:
> > On Wed, 18 Oct 2017 10:47:59 +0200
> > Martin Pieuchot <[hidden email]> wrote:
> >
> > > On 16/10/17(Mon) 14:37, Helg Bredow wrote:
> > > > [...]
> > > > Patch is below.
> > >
> > > Do you mind explaining the problem and the solution you implemented?
> > >
> >
> > There is a regression introduced in 6.2 that can cause a call to getcwd(3) to result in an endless loop. fusefs_readdir() has a TODO that returns success even if a directory is not open, so getcwd(3) keeps calling it to fetch more dirents but none ever get supplied. This patch opens and closes the directory if it's not already open. The check for an invalid file handle was inside the loop but I've moved it outside since it's not going to become invalid once it's been opened.
>
> Nice.  You can remove a call to fb_delete() in the first error path.
>
> > This doesn't resolve the problem however. This is because ifuse_ops_readdir() does not support being called across multiple calls to VOP_READDIR(9). This occurs if the buffer is too small to contain all the entries and getcwd(3) only supplies a 512 byte buffer. If the current directory is not returned in the first call to VOP_READDIR(9) then getcwd(3) will continue to call it but the eofflag is never set. ifuse_ops_readdir() now correctly starts scanning at the next dirent even across multiple VOP_READDIR(9) calls.
>
> Why did you decide to put the logic in the library?  Now the library
> needs to remember some "state".

The logic was already there but it wasn't correct for all cases. The
library also maintained state and I haven't introduced any new state
management variables; I only adjusted how they are maintained.

>
> Would it be possible to keep a dumb ifuse_ops_readdir() and make the
> kernel ask for the next dirent?  How often does that happen in
> practise?  Only when strlen(dirent) > 512?
>

It may be possible to move some of the logic to the kernel, but not
much. Note that the library doesn't maintain state across VOP_READDIR
(9) calls; it only maintains state between kernel requests to keep
filling the fbuf. The change I made is that the library now properly
starts scanning from the supplied offset rather than from 0 every time.
The situation happens whenever the buffer supplied to VOP_READDIR(9) is
too small to contain the entire list of dirents.

> > In addition, if the entries don't evenly fit into the buffer then fusefs_readdir() would prematurely detect that the entire directory had been scanned and set the eofflag. This fix handles this condition so the entire directory is always scanned.
>
> That's good.
>
> > ifuse_fill_readdir() returns the ino of the file system instead of its own ino, which it returns from other calls such as stat(2). getcwd(3) compares these inos when scanning the directory and will never find a match since they are sourced from different file systems. fuse has the use_ino option to allow a file system to use its own inos but this is not implemented in OpenBSD yet. This patch forces the fuse ino to be used in ifuse_fill_readdir(). This is still only paritally implemented though, it will only set a valid ino if it can find the file or directory in the name cache. This is sufficient for getcwd(3) since the parent directory will always be in the cache.
>
> How much work would it be to support the `use_ino' option?
>

I don't think it would be difficult to add support for 'use_ino'. It's
on my list. The OpenBSD implementation will always behave as if the
'readdir_ino' option was specified.

Below is the updated diff without the redundant fb_delete().


Index: lib/libfuse/fuse_ops.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
retrieving revision 1.26
diff -u -p -r1.26 fuse_ops.c
--- lib/libfuse/fuse_ops.c 7 Sep 2016 17:53:35 -0000 1.26
+++ lib/libfuse/fuse_ops.c 29 Oct 2017 13:54:35 -0000
@@ -217,12 +217,15 @@ static int
 ifuse_fill_readdir(void *dh, const char *name, const struct stat
*stbuf, off_t off)
 {
+ struct fuse *f;
  struct fuse_dirhandle *fd = dh;
+ struct fuse_vnode *v;
  struct fusebuf *fbuf;
  struct dirent *dir;
  uint32_t namelen;
  uint32_t len;
 
+ f = fd->fuse;
  fbuf = fd->buf;
  namelen = strnlen(name, MAXNAMLEN);
  len = GENERIC_DIRSIZ(namelen);
@@ -242,13 +245,21 @@ ifuse_fill_readdir(void *dh, const char
  if (off)
  fd->filled = 0;
 
- if (stbuf) {
- dir->d_fileno = stbuf->st_ino;
+ /* TODO Add support for use_ino and readdir_ino */
+ v = get_vn_by_name_and_parent(f, (uint8_t *)name,
fbuf->fb_ino);
+ if (v == NULL) {
+ if (strcmp(name, ".") == 0)
+ dir->d_fileno = fbuf->fb_ino;
+ else
+ dir->d_fileno = 0xffffffff;
+ } else
+ dir->d_fileno = v->ino;
+
+ if (stbuf)
  dir->d_type = IFTODT(stbuf->st_mode);
- } else {
- dir->d_fileno = 0xffffffff;
+ else
  dir->d_type = DT_UNKNOWN;
- }
+
  dir->d_reclen = len;
  dir->d_off = off + len; /* XXX */
  strlcpy(dir->d_name, name, sizeof(dir->d_name));
@@ -295,7 +306,7 @@ ifuse_ops_readdir(struct fuse *f, struct
  ffi.fh = fbuf->fb_io_fd;
  offset = fbuf->fb_io_off;
  size = fbuf->fb_io_len;
- startsave = 0;
+ startsave = offset;
 
  fbuf->fb_dat = calloc(1, size);
 
@@ -314,11 +325,12 @@ ifuse_ops_readdir(struct fuse *f, struct
  if (!vn->fd->filled) {
  vn->fd->filler = ifuse_fill_readdir;
  vn->fd->buf = fbuf;
- vn->fd->filled = 0;
  vn->fd->full = 0;
  vn->fd->size = size;
  vn->fd->off = offset;
  vn->fd->idx = 0;
+ vn->fd->fuse = f;
+ vn->fd->start = offset;
  startsave = vn->fd->start;
 
  realname = build_realname(f, vn->ino);
@@ -345,7 +357,8 @@ ifuse_ops_readdir(struct fuse *f, struct
  if (fbuf->fb_err) {
  fbuf->fb_len = 0;
  vn->fd->filled = 1;
- }
+ } else if (vn->fd->full && fbuf->fb_len == 0)
+ fbuf->fb_err = -ENOBUFS;
 
  if (fbuf->fb_len == 0)
  free(fbuf->fb_dat);
Index: sys/miscfs/fuse/fuse_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.33
diff -u -p -r1.33 fuse_vnops.c
--- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35
-0000 1.33 +++ sys/miscfs/fuse/fuse_vnops.c 29 Oct 2017
13:54:35 -0000 @@ -680,7 +680,7 @@ fusefs_readdir(void *v)
  struct vnode *vp;
  struct proc *p;
  struct uio *uio;
- int error = 0, eofflag = 0;
+ int error = 0, eofflag = 0, diropen = 0;
 
  vp = ap->a_vp;
  uio = ap->a_uio;
@@ -695,14 +695,17 @@ fusefs_readdir(void *v)
  if (uio->uio_resid < sizeof(struct dirent))
  return (EINVAL);
 
+ if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
+ error = fusefs_file_open(fmp, ip, FUFH_RDONLY,
O_RDONLY, 1, p);
+ if (error)
+ return (error);
+
+ diropen = 1;
+ }
+
  while (uio->uio_resid > 0) {
  fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR,
p);
- if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
- /* TODO open the file */
- fb_delete(fbuf);
- return (error);
- }
  fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
  fbuf->fb_io_off = uio->uio_offset;
  fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
@@ -710,6 +713,13 @@ fusefs_readdir(void *v)
  error = fb_queue(fmp->dev, fbuf);
 
  if (error) {
+ /*
+ * dirent was larger than residual space left
in
+ * buffer.
+ */
+ if (error == ENOBUFS && fbuf->fb_len == 0)
+ error = 0;
+
  fb_delete(fbuf);
  break;
  }
@@ -731,6 +741,9 @@ fusefs_readdir(void *v)
 
  if (!error && ap->a_eofflag != NULL)
  *ap->a_eofflag = eofflag;
+
+ if (diropen)
+ fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1,
p);
  return (error);
 }