From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Vivek Goyal <vgoyal@redhat.com> Cc: virtio-fs@redhat.com, mszeredi@redhat.com, lersek@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Subject: Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush() Date: Fri, 11 Dec 2020 19:54:14 +0000 [thread overview] Message-ID: <20201211195414.GG3380@work-vm> (raw) In-Reply-To: <20201211142544.GB3285@redhat.com> * 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
WARNING: multiple messages have this Message-ID (diff)
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Vivek Goyal <vgoyal@redhat.com> Cc: virtio-fs@redhat.com, lersek@redhat.com, qemu-devel@nongnu.org Subject: Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush() Date: Fri, 11 Dec 2020 19:54:14 +0000 [thread overview] Message-ID: <20201211195414.GG3380@work-vm> (raw) In-Reply-To: <20201211142544.GB3285@redhat.com> * 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
next prev parent reply other threads:[~2020-12-11 20:03 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-07 18:30 [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init Vivek Goyal 2020-12-07 18:30 ` [Virtio-fs] " Vivek Goyal 2020-12-07 18:30 ` [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode Vivek Goyal 2020-12-07 18:30 ` [Virtio-fs] " Vivek Goyal 2020-12-07 19:55 ` Vivek Goyal 2020-12-07 19:55 ` [Virtio-fs] " Vivek Goyal 2020-12-10 19:50 ` Dr. David Alan Gilbert 2020-12-10 19:50 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-12-07 18:30 ` [PATCH 2/3] virtiofsd: Disable posix_lock hash table if remote locks are not enabled Vivek Goyal 2020-12-07 18:30 ` [Virtio-fs] " Vivek Goyal 2020-12-10 19:58 ` Dr. David Alan Gilbert 2020-12-10 19:58 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-12-07 18:30 ` [PATCH 3/3] virtiofsd: Check file type in lo_flush() Vivek Goyal 2020-12-07 18:30 ` [Virtio-fs] " Vivek Goyal 2020-12-10 20:03 ` Dr. David Alan Gilbert 2020-12-10 20:03 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-12-10 20:09 ` Vivek Goyal 2020-12-10 20:09 ` [Virtio-fs] " Vivek Goyal 2020-12-10 20:14 ` Dr. David Alan Gilbert 2020-12-10 20:14 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-12-11 14:25 ` Vivek Goyal 2020-12-11 14:25 ` [Virtio-fs] " Vivek Goyal 2020-12-11 19:54 ` Dr. David Alan Gilbert [this message] 2020-12-11 19:54 ` Dr. David Alan Gilbert 2020-12-10 21:24 ` ceph + freeipa ubuntu/fedora common small bug Harry G. Coin 2020-12-10 21:24 ` [Virtio-fs] " Harry G. Coin 2020-12-11 11:05 ` Dr. David Alan Gilbert 2020-12-11 11:05 ` [Virtio-fs] " Dr. David Alan Gilbert 2020-12-11 15:06 ` Vivek Goyal 2020-12-11 15:06 ` [Virtio-fs] " Vivek Goyal 2020-12-12 6:39 ` Harry Coin 2020-12-12 6:39 ` [Virtio-fs] " Harry Coin 2020-12-07 19:12 ` [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init no-reply 2020-12-07 19:12 ` [Virtio-fs] " no-reply 2020-12-08 4:51 ` Laszlo Ersek 2020-12-08 4:51 ` [Virtio-fs] " Laszlo Ersek 2020-12-08 14:16 ` Vivek Goyal 2020-12-08 14:16 ` [Virtio-fs] " Vivek Goyal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201211195414.GG3380@work-vm \ --to=dgilbert@redhat.com \ --cc=lersek@redhat.com \ --cc=mszeredi@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=vgoyal@redhat.com \ --cc=virtio-fs@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.