Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Dec 10, 2020 at 08:14:31PM +0000, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > > Currently lo_flush() is written in such a way that it expects to 
> > > > > receive
> > > > > a FLUSH requests on a regular file (and not directories). For example,
> > > > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > > > using opendir(), we keep don't keep track of these in lo->fd_map 
> > > > > instead
> > > > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > > > regular files only.
> > > > > 
> > > > > Even linux fuse client calls FLUSH only for regular files and not
> > > > > directories. So put a check for filetype and return EBADF if
> > > > > lo_flush() is called on a non-regular file.
> > > > > 
> > > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > > ---
> > > > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/virtiofsd/passthrough_ll.c 
> > > > > b/tools/virtiofsd/passthrough_ll.c
> > > > > index 8ba79f503a..48a109d3f6 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t 
> > > > > ino, struct fuse_file_info *fi)
> > > > >      struct lo_data *lo = lo_data(req);
> > > > >  
> > > > >      inode = lo_inode(req, ino);
> > > > > -    if (!inode) {
> > > > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > > > >          fuse_reply_err(req, EBADF);
> > > > 
> > > > Does that need a lo_inode_put(lo, &inode) in the new case?
> > > 
> > > Good catch. Yes if inode is valid but file type is not regular, we need
> > > to put inode reference.
> > > 
> > > Do you want me to post a new patch or you will like to take care of
> > > it.
> > 
> > OK, so if we make this :
> > 
> >   if (!inode) {
> >       fuse_reply_err(req, EBADF);
> >       return;
> >   }
> > 
> >   if (!S_ISREG(inode->filetype)) {
> >       lo_inode_put(lo_data(req), &inode);
> >       fuse_reply_err(req, EBADF);
> >       return;
> >   }
> > 
> > (Untested)
> 
> Hi Dave,
> 
> Above looks good. For your convenience, I updated the patch and also
> tested it by running blogbench and things look fine.
> 
> Vivek
> 
> Subject: virtiofsd: Check file type in lo_flush()
> 
> Currently lo_flush() is written in such a way that it expects to receive
> a FLUSH requests on a regular file (and not directories). For example,
> we call lo_fi_fd() which searches lo->fd_map. If we open directories
> using opendir(), we keep don't keep track of these in lo->fd_map instead
> we keep them in lo->dir_map. So we expect lo_flush() to be called on
> regular files only.
> 
> Even linux fuse client calls FLUSH only for regular files and not
> directories. So put a check for filetype and return EBADF if 
> lo_flush() is called on a non-regular file.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

OK< thanks

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and queued.
(I've dropped the Tested-by since it's had a bit of forceful rework).

> ---
>  tools/virtiofsd/passthrough_ll.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c       2020-12-11 
> 09:00:28.787669761 -0500
> +++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c    2020-12-11 
> 09:03:38.239496505 -0500
> @@ -1973,6 +1973,12 @@ static void lo_flush(fuse_req_t req, fus
>          return;
>      }
>  
> +    if (!S_ISREG(inode->filetype)) {
> +        lo_inode_put(lo, &inode);
> +        fuse_reply_err(req, EBADF);
> +        return;
> +    }
> +
>      /* An fd is going away. Cleanup associated posix locks */
>      if (lo->posix_lock) {
>          pthread_mutex_lock(&inode->plock_mutex);
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK