Re: [PATCH v2 03/29] tools/xenlogd: connect to frontend

[Juergen Gross]

On 20.11.23 16:49, Jason Andryuk wrote:

On Fri, Nov 10, 2023 at 1:04 PM Juergen Gross <jgr...@suse.com> wrote:

Add the code for connecting to frontends to xenlogd.

Signed-off-by: Juergen Gross <jgr...@suse.com>

diff --git a/tools/xen-9pfsd/xen-9pfsd.c b/tools/xen-9pfsd/xen-9pfsd.c
index c365b35fe5..cc5734402d 100644
--- a/tools/xen-9pfsd/xen-9pfsd.c
+++ b/tools/xen-9pfsd/xen-9pfsd.c

+static int check_host_path(device *device)
+{
+    struct stat statbuf;
+    char *path, *p;
+    int ret = 1;
+
+    if ( !device->host_path )
+        return 1;
+
+    if ( device->host_path[0] != '/' )
+        return 1;
+

 From v1, you stated for alloc_fid_mem(device, fid, path):

No, "path" is always starting with a "/" if it is not empty.

And then
snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);

While alloc_fid_mem() uses "%s%s"

And p9_create() uses "%s/%s"

Of course it does, as this is the concatenation of the current path with
the new file name, which is relative to the current path.

p9_walk does:
const char *rel_path = path + strlen(device->host_path)
...
alloc_fid_mem(device, fid, rel_path);

So host_path is expected not to have a tailing '/' to ensure that
rel_path starts with a '/'.  So you want to error out for a trailing
'/' (or overwrite with '\0')?

I can add that.

It seems like alloc_fid_mem() should also check to ensure path is "'/'
if it is not empty".

I'll add an assert().

This is all subtle and security relevant, so it's important to get
this right.  A code comment explaining the expectation of paths for
host_path vs. fids would be good.

Agreed.

Also, maybe using openat() would be a better approach?  Create the
dirfd pointing at the 9pfs export and then use relative paths for the
paths inside.  This would cut down on the manual path manipulations.

I'll look into that. I'll have to trade special casing accessing the root
directory vs. reducing path operations.

+    path = strdup(device->host_path);
+    if ( !path )
+    {
+        syslog(LOG_CRIT, "memory allocation failure!");
+        return 1;
+    }
+
+    for ( p = path; p; )
+    {
+        p = strchr(p + 1, '/');
+        if ( p )
+            *p = 0;
+        if ( !stat(path, &statbuf) )
+        {
+            if ( !(statbuf.st_mode & S_IFDIR) )
+                break;
+            if ( !p )
+            {
+                ret = 0;
+                break;
+            }
+            *p = '/';
+            continue;
+        }
+        if ( mkdir(path, 0777) )
+            break;
+        if ( p )
+            *p = '/';
+    }
+
+    free(path);
+    return ret;
+}
+

+
+static int write_backend_node(device *device, const char *node, const char 
*val)
+{
+    struct path p;
+    struct xs_permissions perms[2] = {
+        { .id = 0, .perms = XS_PERM_NONE },

This hard codes dom0.  If xs_permissions supported DOMID_SELF, it
wouldn't need to be looked up.

DOMID_SELF isn't supported. But you are right, I need to avoid hard coding dom0
here. This can be achieved by reading the permissions after creating the node
and use the result for the first permission entry.

+        { .id = device->domid, .perms = XS_PERM_READ }
+    };
+
+    construct_backend_path(device, node, &p);
+    if ( !xs_write(xs, XBT_NULL, p.path, val, strlen(val)) )
+    {
+        syslog(LOG_ERR, "error writing bacḱend node \"%s\" for device %u/%u",
+               node, device->domid, device->devid);
+        return 1;
+    }
+
+    if ( !xs_set_permissions(xs, XBT_NULL, p.path, perms, 2) )
+    {
+        syslog(LOG_ERR, "error setting permissions for \"%s\"", p.path);
+        return 1;
+    }
+
+    return 0;
+}
+

+
+static void connect_device(device *device)
+{
+    unsigned int val;
+    unsigned int ring_idx;
+    char node[20];
+    struct ring *ring;
+    xenevtchn_port_or_error_t evtchn;
+
+    val = read_frontend_node_uint(device, "version", 0);
+    if ( val != 1 )
+        return connect_err(device, "frontend specifies illegal version");
+    device->num_rings = read_frontend_node_uint(device, "num-rings", 0);
+    if ( device->num_rings < 1 || device->num_rings > MAX_RINGS )
+        return connect_err(device, "frontend specifies illegal ring number");
+
+    for ( ring_idx = 0; ring_idx < device->num_rings; ring_idx++ )
+    {
+        ring = calloc(1, sizeof(*ring));
+        if ( !ring )
+            return connect_err(device, "could not allocate ring memory");
+        device->ring[ring_idx] = ring;
+        ring->device = device;
+        pthread_cond_init(&ring->cond, NULL);
+        pthread_mutex_init(&ring->mutex, NULL);
+
+

extra blank line.

Will remove it.


Thanks,

Juergen

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature