diff mbox series

[10/24] DAX: virtiofsd: Add setup/remove mappings fuse commands

Message ID 20210209190224.62827-11-dgilbert@redhat.com
State New
Headers show
Series virtiofs dax patches | expand

Commit Message

Dr. David Alan Gilbert Feb. 9, 2021, 7:02 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add commands so that the guest kernel can ask the daemon to map file
sections into a guest kernel visible cache.

Note: Catherine Ho had sent a patch to fix an issue with multiple
removemapping. It was a merge issue though.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com>
Including-fixes: Catherine Ho <catherine.hecx@gmail.com>
Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
---
 tools/virtiofsd/fuse_lowlevel.c | 68 +++++++++++++++++++++++++++++++++
 tools/virtiofsd/fuse_lowlevel.h | 23 ++++++++++-
 2 files changed, 90 insertions(+), 1 deletion(-)

Comments

Stefan Hajnoczi Feb. 11, 2021, 12:37 p.m. UTC | #1 
On Tue, Feb 09, 2021 at 07:02:10PM +0000, Dr. David Alan Gilbert (git) wrote:
> +static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> +                             struct fuse_mbuf_iter *iter)
> +{
> +    struct fuse_removemapping_in *arg;
> +    struct fuse_removemapping_one *one;
> +
> +    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> +    if (!arg || arg->count <= 0) {

arg->count is unsigned so < is tautologous.

> +        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> +        fuse_reply_err(req, EINVAL);
> +        return;
> +    }
> +
> +    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));

arg->count * sizeof(*one) is an integer overflow on 32-bit hosts. I
think we should be more defensive here since this input comes from the
guest.
Dr. David Alan Gilbert Feb. 11, 2021, 4:39 p.m. UTC | #2 
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Tue, Feb 09, 2021 at 07:02:10PM +0000, Dr. David Alan Gilbert (git) wrote:
> > +static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > +                             struct fuse_mbuf_iter *iter)
> > +{
> > +    struct fuse_removemapping_in *arg;
> > +    struct fuse_removemapping_one *one;
> > +
> > +    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > +    if (!arg || arg->count <= 0) {
> 
> arg->count is unsigned so < is tautologous.
> 
> > +        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> > +        fuse_reply_err(req, EINVAL);
> > +        return;
> > +    }
> > +
> > +    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));
> 
> arg->count * sizeof(*one) is an integer overflow on 32-bit hosts. I
> think we should be more defensive here since this input comes from the
> guest.

OK, so I've gone with:

    if (!arg || !arg->count || 
        (uint64_t)arg->count * sizeof(*one) >= SIZE_MAX) {
        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
        fuse_reply_err(req, EINVAL);
        return;
    }

to fix both of those (the compiler likes to moan on 64bit about
that comparison being always false in the simpler ways I tried it).

Dave
Vivek Goyal Feb. 11, 2021, 6:30 p.m. UTC | #3 
On Thu, Feb 11, 2021 at 04:39:22PM +0000, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > On Tue, Feb 09, 2021 at 07:02:10PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > +static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > > +                             struct fuse_mbuf_iter *iter)
> > > +{
> > > +    struct fuse_removemapping_in *arg;
> > > +    struct fuse_removemapping_one *one;
> > > +
> > > +    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > +    if (!arg || arg->count <= 0) {
> > 
> > arg->count is unsigned so < is tautologous.
> > 
> > > +        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> > > +        fuse_reply_err(req, EINVAL);
> > > +        return;
> > > +    }
> > > +
> > > +    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));
> > 
> > arg->count * sizeof(*one) is an integer overflow on 32-bit hosts. I
> > think we should be more defensive here since this input comes from the
> > guest.
> 
> OK, so I've gone with:
> 
>     if (!arg || !arg->count || 
>         (uint64_t)arg->count * sizeof(*one) >= SIZE_MAX) {
>         fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
>         fuse_reply_err(req, EINVAL);
>         return;

If we did not want to get into unit64_t business, can we alternatively do.
     if (!arg || !arg->count || arg->count > SIZE_MAX/sizeof(*one)) {
     }

Vivek
Dr. David Alan Gilbert Feb. 11, 2021, 7:50 p.m. UTC | #4 
* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Feb 11, 2021 at 04:39:22PM +0000, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > On Tue, Feb 09, 2021 at 07:02:10PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > > +static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > > > +                             struct fuse_mbuf_iter *iter)
> > > > +{
> > > > +    struct fuse_removemapping_in *arg;
> > > > +    struct fuse_removemapping_one *one;
> > > > +
> > > > +    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > > +    if (!arg || arg->count <= 0) {
> > > 
> > > arg->count is unsigned so < is tautologous.
> > > 
> > > > +        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> > > > +        fuse_reply_err(req, EINVAL);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));
> > > 
> > > arg->count * sizeof(*one) is an integer overflow on 32-bit hosts. I
> > > think we should be more defensive here since this input comes from the
> > > guest.
> > 
> > OK, so I've gone with:
> > 
> >     if (!arg || !arg->count || 
> >         (uint64_t)arg->count * sizeof(*one) >= SIZE_MAX) {
> >         fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> >         fuse_reply_err(req, EINVAL);
> >         return;
> 
> If we did not want to get into unit64_t business, can we alternatively do.
>      if (!arg || !arg->count || arg->count > SIZE_MAX/sizeof(*one)) {

I tried that and the compiler moaned that it was always false; which on
a 64bit host it is since arg->count is uint32_t.

Dave

>      }
> 
> Vivek
Vivek Goyal Feb. 11, 2021, 8:15 p.m. UTC | #5 
On Thu, Feb 11, 2021 at 07:50:37PM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > On Thu, Feb 11, 2021 at 04:39:22PM +0000, Dr. David Alan Gilbert wrote:
> > > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > > On Tue, Feb 09, 2021 at 07:02:10PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > > > +static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > > > > +                             struct fuse_mbuf_iter *iter)
> > > > > +{
> > > > > +    struct fuse_removemapping_in *arg;
> > > > > +    struct fuse_removemapping_one *one;
> > > > > +
> > > > > +    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > > > +    if (!arg || arg->count <= 0) {
> > > > 
> > > > arg->count is unsigned so < is tautologous.
> > > > 
> > > > > +        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> > > > > +        fuse_reply_err(req, EINVAL);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));
> > > > 
> > > > arg->count * sizeof(*one) is an integer overflow on 32-bit hosts. I
> > > > think we should be more defensive here since this input comes from the
> > > > guest.
> > > 
> > > OK, so I've gone with:
> > > 
> > >     if (!arg || !arg->count || 
> > >         (uint64_t)arg->count * sizeof(*one) >= SIZE_MAX) {
> > >         fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
> > >         fuse_reply_err(req, EINVAL);
> > >         return;
> > 
> > If we did not want to get into unit64_t business, can we alternatively do.
> >      if (!arg || !arg->count || arg->count > SIZE_MAX/sizeof(*one)) {
> 
> I tried that and the compiler moaned that it was always false; which on
> a 64bit host it is since arg->count is uint32_t.

Hmm.... May be something like.

bool is_arg_count_valid()
{
  if (!arg->count)
      return false;

#if __WORDSIZE == 64
   return true;
#elif
  if (argc->count > SIZE_MAX/sizeof(*one))
      return false;
#fi
  return true;
}

if (!argc || !is_arg_count_valie()) {
}

Vivek
diff mbox series

Patch

diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index e94b71110b..0d3768b7d0 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -1868,6 +1868,72 @@  static void do_lseek(fuse_req_t req, fuse_ino_t nodeid,
     }
 }
 
+static void do_setupmapping(fuse_req_t req, fuse_ino_t nodeid,
+                            struct fuse_mbuf_iter *iter)
+{
+    struct fuse_setupmapping_in *arg;
+    struct fuse_file_info fi;
+
+    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+    if (!arg) {
+        fuse_reply_err(req, EINVAL);
+        return;
+    }
+
+    memset(&fi, 0, sizeof(fi));
+    fi.fh = arg->fh;
+
+    /*
+     *  TODO: Need to come up with a better definition of flags here; it can't
+     * be the kernel view of the flags, since that's abstracted from the client
+     * similarly, it's not the vhost-user set
+     * for now just use O_ flags
+     */
+    uint64_t genflags;
+
+    genflags = O_RDONLY;
+    if (arg->flags & FUSE_SETUPMAPPING_FLAG_WRITE) {
+        genflags = O_RDWR;
+    }
+
+    if (req->se->op.setupmapping) {
+        req->se->op.setupmapping(req, nodeid, arg->foffset, arg->len,
+                                 arg->moffset, genflags, &fi);
+    } else {
+        fuse_reply_err(req, ENOSYS);
+    }
+}
+
+static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
+                             struct fuse_mbuf_iter *iter)
+{
+    struct fuse_removemapping_in *arg;
+    struct fuse_removemapping_one *one;
+
+    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
+    if (!arg || arg->count <= 0) {
+        fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid arg %p\n", arg);
+        fuse_reply_err(req, EINVAL);
+        return;
+    }
+
+    one = fuse_mbuf_iter_advance(iter, arg->count * sizeof(*one));
+    if (!one) {
+        fuse_log(
+            FUSE_LOG_ERR,
+            "do_removemapping: invalid in, expected %d * %ld, has %ld - %ld\n",
+            arg->count, sizeof(*one), iter->size, iter->pos);
+        fuse_reply_err(req, EINVAL);
+        return;
+    }
+
+    if (req->se->op.removemapping) {
+        req->se->op.removemapping(req, req->se, nodeid, arg->count, one);
+    } else {
+        fuse_reply_err(req, ENOSYS);
+    }
+}
+
 static void do_init(fuse_req_t req, fuse_ino_t nodeid,
                     struct fuse_mbuf_iter *iter)
 {
@@ -2258,6 +2324,8 @@  static struct {
     [FUSE_RENAME2] = { do_rename2, "RENAME2" },
     [FUSE_COPY_FILE_RANGE] = { do_copy_file_range, "COPY_FILE_RANGE" },
     [FUSE_LSEEK] = { do_lseek, "LSEEK" },
+    [FUSE_SETUPMAPPING] = { do_setupmapping, "SETUPMAPPING" },
+    [FUSE_REMOVEMAPPING] = { do_removemapping, "REMOVEMAPPING" },
 };
 
 #define FUSE_MAXOP (sizeof(fuse_ll_ops) / sizeof(fuse_ll_ops[0]))
diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
index c0ff4f07a4..014564ff07 100644
--- a/tools/virtiofsd/fuse_lowlevel.h
+++ b/tools/virtiofsd/fuse_lowlevel.h
@@ -24,6 +24,7 @@ 
 #endif
 
 #include "fuse_common.h"
+#include "standard-headers/linux/fuse.h"
 
 #include <sys/statvfs.h>
 #include <sys/uio.h>
@@ -1170,7 +1171,6 @@  struct fuse_lowlevel_ops {
      */
     void (*readdirplus)(fuse_req_t req, fuse_ino_t ino, size_t size, off_t off,
                         struct fuse_file_info *fi);
-
     /**
      * Copy a range of data from one file to another
      *
@@ -1226,6 +1226,27 @@  struct fuse_lowlevel_ops {
      */
     void (*lseek)(fuse_req_t req, fuse_ino_t ino, off_t off, int whence,
                   struct fuse_file_info *fi);
+
+    /*
+     * Map file sections into kernel visible cache
+     *
+     * Map a section of the file into address space visible to the kernel
+     * mounting the filesystem.
+     * TODO
+     */
+    void (*setupmapping)(fuse_req_t req, fuse_ino_t ino, uint64_t foffset,
+                         uint64_t len, uint64_t moffset, uint64_t flags,
+                         struct fuse_file_info *fi);
+
+    /*
+     * Unmap file sections in kernel visible cache
+     *
+     * Unmap sections previously mapped by setupmapping
+     * TODO
+     */
+    void (*removemapping)(fuse_req_t req, struct fuse_session *se,
+                          fuse_ino_t ino, unsigned num,
+                          struct fuse_removemapping_one *argp);
 };
 
 /**