| Message ID | 20201207183021.22752-4-vgoyal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofsd: Fix lo_flush() and inode->posix_lock init | expand |
* 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? Dave > return; > } > -- > 2.25.4 >
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. Vivek
* 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) Dave > > Vivek
FYI. Same thing we saw on Fedora installing freeipa, this on ubuntu with ceph. Identical bitmask report. ... Fixing /var/run/ceph ownership....done Cannot set file attribute for '/var/log/journal', value=0x00800000, mask=0x00800000, ignoring: Function not implemented Cannot set file attribute for '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000, mask=0x00800000, ignoring: Function not implemented ... Host has xattrs on, btrfs.
* Harry G. Coin (hgcoin@gmail.com) wrote: > FYI. Same thing we saw on Fedora installing freeipa, this on ubuntu > with ceph. Identical bitmask report. > > ... > > Fixing /var/run/ceph ownership....done > > Cannot set file attribute for '/var/log/journal', value=0x00800000, > mask=0x00800000, ignoring: Function not implemented > > Cannot set file attribute for > '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000, > mask=0x00800000, ignoring: Function not implemented This looks like it comes out of systemd's src/tmpfiles/tmpfiles.c: r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL); if (r < 0) log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING, r, "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m", path, item->attribute_value, item->attribute_mask); and it's chattr_fd is in it's src/basic/chattr-util.c which is using FS_IOC_GET/SETFLAGS, which seems to be an older way of doing things. Now, is that supposed to promote itself to a newer call or is it OK? Dave > ... > > Host has xattrs on, btrfs. > > >
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> --- 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);
On Fri, Dec 11, 2020 at 11:05:22AM +0000, Dr. David Alan Gilbert wrote: > * Harry G. Coin (hgcoin@gmail.com) wrote: > > FYI. Same thing we saw on Fedora installing freeipa, this on ubuntu > > with ceph. Identical bitmask report. > > > > ... > > > > Fixing /var/run/ceph ownership....done > > > > Cannot set file attribute for '/var/log/journal', value=0x00800000, > > mask=0x00800000, ignoring: Function not implemented > > > > Cannot set file attribute for > > '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000, > > mask=0x00800000, ignoring: Function not implemented > > This looks like it comes out of systemd's src/tmpfiles/tmpfiles.c: > > r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL); > if (r < 0) > log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING, > r, > "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m", > path, item->attribute_value, item->attribute_mask); > > and it's chattr_fd is in it's src/basic/chattr-util.c > which is using FS_IOC_GET/SETFLAGS, which seems to be an older > way of doing things. > > Now, is that supposed to promote itself to a newer call or is it OK? I see that we don't have any ->ioctl function registered in passthrough_ll.c and that's why do_ioctl() (fuse_lowlevel.c) will return -ENOSYS. So we probably need to modify passthrough_ll.c to support some select ioctls. Right now it looks like all fs ioctls will return -ENOSYS. I tried "chattr +i foo.txt" and that return -ENOSYS as well. Vivek
* 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);
It's the latest ubuntu and fedora distros. On December 11, 2020 5:05:22 AM CST, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >* Harry G. Coin (hgcoin@gmail.com) wrote: >> FYI. Same thing we saw on Fedora installing freeipa, this on ubuntu >> with ceph. Identical bitmask report. >> >> ... >> >> Fixing /var/run/ceph ownership....done >> >> Cannot set file attribute for '/var/log/journal', value=0x00800000, >> mask=0x00800000, ignoring: Function not implemented >> >> Cannot set file attribute for >> '/var/log/journal/fd007229322043ad8778c214d19ed3ac', >value=0x00800000, >> mask=0x00800000, ignoring: Function not implemented > >This looks like it comes out of systemd's src/tmpfiles/tmpfiles.c: > > r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL); > if (r < 0) >log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : >LOG_WARNING, > r, >"Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, >ignoring: %m", > path, item->attribute_value, item->attribute_mask); > >and it's chattr_fd is in it's src/basic/chattr-util.c >which is using FS_IOC_GET/SETFLAGS, which seems to be an older >way of doing things. > >Now, is that supposed to promote itself to a newer call or is it OK? > >Dave > >> ... >> >> Host has xattrs on, btrfs. >> >> >> >-- >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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); return; }
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(-)